Dirichlet values#74
Conversation
… into dirichlet_values
|
Hi @amartinhuertas , @fverdugo . Is there any missing point that prevents the merge of this PR? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 7 7
Lines 1240 1322 +82
======================================
- Misses 1240 1322 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @abstractmethod | ||
| end | ||
|
|
||
| function get_dirichlet_dof_ids(f::DistributedFESpace) |
There was a problem hiding this comment.
Missing FESpaces., agreed?
There was a problem hiding this comment.
BTW, did you implement this abstract method for the subtypes of DistributedFESpaces? I am not able to see the implementations. If they are not implemented, why arent they required?
There was a problem hiding this comment.
Fixed in 7030e01.
Regarding the implementation for subtypes, that would only involve DistributedMultiFieldFESpace, right? In that case the specialization is not necessary because this function is called for each of the single FE spaces. That's also the case in Gridap, where this function is not specialized for MultiFieldFESpaces.
There was a problem hiding this comment.
I have been taken a more close look at the rationale behind the get_dirichlet_dof_ids in Gridap.jl.
The abstract method at hand is defined at the SingleFieldFESpace level in the type hierarchy.
All the subtypes of SingleFieldFESpace implement it, as expected
julia> subtypes(Gridap.FESpaces.SingleFieldFESpace)
6-element Vector{Any}:
Gridap.FESpaces.DirichletFESpace
Gridap.FESpaces.FESpaceWithConstantFixed
Gridap.FESpaces.FESpaceWithLinearConstraints
Gridap.FESpaces.UnconstrainedFESpace
Gridap.FESpaces.ZeroMeanFESpace
TrialFESpace
The method is called SOLELY within the whole Gridap.jl code at the following two points:
num_dirichlet_dofs(f::SingleFieldFESpace) = length(get_dirichlet_dof_ids(f))
function get_free_dof_ids(f::DirichletFESpace)
get_dirichlet_dof_ids(f.space)
endGoing back to GridapDistributed.jl, and given the above, wouldn't it make more sense to define (and implement) the method for DistributedSingleFieldFESpace? Should we also implement num_dirichlet_dofs?
In any case, I think that before implementing anything we should understand why do we need this method in GridapDistributed. Why do you need it?
There was a problem hiding this comment.
Why do you need it?
Ok, I now see that you need it in the following line (sorry, I had missed it):
Should we also implement num_dirichlet_dofs?
Ok, I see you have implemented it here:
Anyway, i still do not get why it is working. Are you actually calling zero_dirichlet_values(f::DistributedFESpace) ?
There was a problem hiding this comment.
I used zero_dirichlet_values in a driver prior to having the current interpolate_everywhere implementation. It is not used for the devs proposed in this PR and, to be consistent, I removed from this PR. If needed in the future, we can re-implement it.
Hi @oriolcg ! Thanks for your work. I have added some comments to the code you have added. Besides, I also miss a description of the work you did in the NEWS.md file. |
| fill!(vec,zero(eltype(vec))) | ||
| end | ||
|
|
||
| function FESpaces.zero_dirichlet_values(f::DistributedFESpace) |
There was a problem hiding this comment.
I am not sure if it is correct. What is the type of vec ?
I think that the "tricky" part regarding Dirichlet dofs is that we never generate a global numeration for them. Thus, the queries related with dirichlet dofs, should to return an AbstractPData object that wraps the corresponding serial Gridap call. They cannot return global objects that would require to generate a global numeration for them.
Another option would be to create a global numeration for Dirichlet dofs. It would be more elegant because the rationale for free and Dirichlet would be equivalent, but this is an extra cost not needed in general.
There was a problem hiding this comment.
Removed in fa9873e (to be re-implemented in another PR if needed)
|
|
||
| FESpaces.num_free_dofs(f::DistributedFESpace) = length(get_free_dof_ids(f)) | ||
|
|
||
| FESpaces.num_dirichlet_dofs(f::DistributedFESpace) = length(get_dirichlet_dof_ids(f)) |
There was a problem hiding this comment.
I would say that even this one needs to return an AbstractPData object.
4 participants
Added support for
interpolate_everywhere