-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimizing conforming rt fes #638
Conversation
function get_cell_shapefuns(model::DiscreteModel, cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}}, ::DivConformity) as per fverdugo comments.
Codecov Report
@@ Coverage Diff @@
## master #638 +/- ##
==========================================
- Coverage 87.70% 87.70% -0.01%
==========================================
Files 134 134
Lines 14315 14320 +5
==========================================
+ Hits 12555 12559 +4
- Misses 1760 1761 +1
Continue to review full report at Codecov.
|
Hi @fverdugo ... the tests in this PR failed due to VERY strange error: https://github.com/gridap/Gridap.jl/pull/638/checks?check_run_id=3321610616#step:6:131 In particular, the symbol Can it bit some sort of dirty cache in Github actions or something like that? Have you ever find something similar? |
…conforming_rt_fes
- Some fixes to DIV pending from PR #636
OK ... this PR is already for review, up to solving the following: |
checks are not displayed. Perhaps a dummy commit to re-run them? |
Ok. I have pushed a new commit and the issue seems to have disappear ... weird ! |
…conforming_rt_fes
function get_cell_dof_basis(model::DiscreteModel, cell_reffe::AbstractArray{<:GenericRefFE{RaviartThomas}}, ::DivConformity) To this end: (1) Replaced a function by a new Map instance. (2) I early evaluated the Jacobian at all integration points (instead of on a cell by cell basis). Did not observe signficant performance improvements, though.
@fverdugo ... FYI ... this PR is ready for review |
src/Fields/ArrayBlocks.jl
Outdated
@@ -296,6 +296,14 @@ function lazy_map(::typeof(evaluate),a::LazyArray{<:Fill{<:BlockMap}},x::Abstrac | |||
lazy_map(k,args...) | |||
end | |||
|
|||
function lazy_map(k::Broadcasting,a::LazyArray{<:Fill{<:BlockMap}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only part that I am not sure about in this PR.
Perhaps replacing Broadcasting
by BroadcastingFieldOpMap
makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only part that I am not sure about in this PR.
Ok, this is what I was also wondering; see #637 (comment)
Perhaps replacing Broadcasting by BroadcastingFieldOpMap makes more sense.
To put you in context (if you were not already), this lazy_map
overload is currently triggered from this line
If I understand correctly, you want to defer this "change of order" of operations till evaluation, right? Apart from consistency with other parts of the code, what are the benefit of this? (for my understanding).
Also @fverdugo ... do u have any insight on this question? #637 (comment) |
My concern is that this function lazy_map(k::Broadcasting,a::LazyArray{<:Fill{<:BlockMap}})
args = map(i->lazy_map(k,i),a.args)
bm = a.maps.value
lazy_map(bm,args...)
end is only valid when Unfortunately. I would say that
is not "multifield" yet. Thus, ArrayBlock and BlockMap should not be involved here.
If |
Assembly routines can allocate large amounts of memory (e.g. intermediate sparse representations of your matrix). The explicit call to In any case, after commenting the Do you know what is causing this? perhaps is also related with |
Ok, this is what I thought. Did you observe the positive impact of gc in memory consumption? Have you studied this in detail? Perhaps (this is speculation, I am not an expert in Julia internals) by forcing the gc to enter in a particular moment in time, instead of letting it decide when it is best to enter, is causing performance issues.
Ok, that is a good observation. This plateu was also present in the flame graphs previous to deactivating gc and occurs during the solve stage. As you say, looking at the RT solve column before and after gc, there has been an increase of the computational time of RT solve, but this is increase is not as significant as the one in Assembly when we have explicit gc. |
Looking at the Total column helps in determining the best compromise. |
by "linear" you mean k(x+y)=k(x)+k(y) and k(alphax)=alphak(x) ? I do not see the relantionship among being linear in this sense and the impossibility/uncorrectness of defining the result of broadcasting an operation to a vector block as the vector block of broadcasting the operation to its blocks. (Sure I am missing something.)
Ok, let me explore this and I will come back to you if I find a solution. |
If k is not linear linear, i.e., k(ax) != ak(x), zero blocks in the input can result in non-zero blocks in the output. |
…conforming_rt_fes
@fverdugo ... I have been thinking carefully about this comment. While we may try to achieve this, let me stress that the comment is NOT consistent with what Gridap is currently doing in analagous situations. To illustrate my point, I wrote the following MWE: using Gridap
using Gridap.CellData
n = 3
domain = (0,1,0,1)
partition = (n,n)
model = simplexify(CartesianDiscreteModel(domain, partition))
order = 1
reffeᵤ = ReferenceFE(lagrangian,Float64,order)
reffeₚ = ReferenceFE(lagrangian,Float64,order)
V = TestFESpace(model,reffeᵤ,conformity=:H1)
Q = TestFESpace(model,reffeₚ,conformity=:H1)
Y = MultiFieldFESpace([V,Q])
v=get_fe_basis(V)
y=get_fe_basis(Y)
vb,_=y
∇(vb) and debugged carefully the last line. This line triggers the following line: Gridap.jl/src/CellData/CellFields.jl Line 428 in f1ad186
which in turns triggers: Gridap.jl/src/Fields/ArrayBlocks.jl Line 459 in f1ad186
Perhaps I am missing something, but I do not see how come this is different from what I am trying to achieve with the method dispatch I have introduced. The difference among Gridap.jl/src/CellData/CellFields.jl Line 428 in f1ad186
and my particular case, is that in the former the array to which we apply the
See my comment above. Isn't Gridap.jl/src/Fields/ArrayBlocks.jl Line 459 in f1ad186
violating the property you are referring to? |
Ok. I have now realized (I have run the debugger to double check) that this statement I am quoting is WRONG (sorry about that!). In particular, the new Gridap.jl/src/CellData/CellFields.jl Line 428 in f1ad186
Here |
Ok I see! Then we can add the new method overload you proposed but restricted to the gradient to be on the safe side (i.e. to avoid using it with non linear kernels function lazy_map(k::Broadcasting{typeof(gradient)},a::LazyArray{<:Fill{<:BlockMap}})
args = map(i->lazy_map(k,i),a.args)
bm = a.maps.value
lazy_map(bm,args...)
end |
…conforming_rt_fes
Broadcasting{typeof(gradient)} kernel
Ok, deal. Is there any example of a non-linear kernel in Gridap.jl used in conjunction with I want to make sure I have understood the point. (to avoid introducing the same issue in the future).
Yes. (I have double checked). It remains to decide what to do with the explicit call to GC(). It is currently commented in this PR. |
We can remove the call to |
I think so. It can be merged. |
…conforming_rt_fes
see issue #637