Skip to content
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

Add modal c0 machinery #777

Merged
merged 9 commits into from
May 4, 2022
Merged

Add modal c0 machinery #777

merged 9 commits into from
May 4, 2022

Conversation

ericneiva
Copy link
Member

Hi, @santiagobadia,

This PR adds (generalised) Modal C0 bases and reffes into Gridap.

I have refactored the code such that it is minimally invasive.

There are two things however to improve:

  1. Modal C0 reffes are generally different at each cell, because they express different shape functions according to the cell bounding box. In the implementation, I am assigning a different Modal C0 cell type to each cell. On the other hand, the cell-wise ownership given by CellConformity is compressed on cell types. At the moment, I compute the CellConformity for each Modal C0 cell type. This is redundant. We have different shape functions, but the CellConformity is usually the same everywhere. I have found out that this approach is quite inefficient in
    function CellConformity(cell_reffe::AbstractArray{<:ReferenceFE},conf::Conformity)
    and
    function _d_ctype_ldface_own_ldofs(a::CellConformity)
    We should definitely compute once (the shared) CellConformity and assign it to all Modal C0 cell types.
  2. In order to reuse the procedure that creates the AgFE space on top of the standard one, I am expressing the Modal C0 shape functions as a trivial linear combination in
    linear_combination(Eye{Int}(ndofs),shapefuns)) # Trick to be able to eval dofs af shapefuns
    If I don't do this I cannot evaluate dofs at root shape functions in the physical domain, as Gridap errors at attempting to evaluate a single Modal C0 shapefun at a single point. I think Gridap's design is planned to evaluate the whole Modal C0 basis at a single point, not a single shapefun. Maybe @fverdugo knows what is missing.

We can discuss about those two points whenever you want.

In the meantime, the new functionality (up to the points above) can be reviewed and merged into the code.

Cheers!

@fverdugo
Copy link
Member

I think Gridap's design is planned to evaluate the whole Modal C0 basis at a single point, not a single shapefun. Maybe @fverdugo knows what is missing.

It is possible (at least in theory) to evaluate a single function at a single point. But this situations is not used often, and thus it is likely that there is missing implementations related with this. If you provide a minimal example that crashes with this error, I can evaluate what is still missing.

@ericneiva
Copy link
Member Author

ericneiva commented Apr 26, 2022

Hi, @fverdugo,

If you run the 1D snippet below checking out on the previous commit of this branch, i.e., 1bf6f6c, you should get the error I am referring to:

using Gridap
using Gridap.CellData
using Gridap.FESpaces

u(x) = x[1]^3

domain = (0,4)
partition = (4,)
model = CartesianDiscreteModel(domain,partition)

order = 3
reffe = ReferenceFE(modalC0,Float64,order)
V = FESpace(model,reffe,conformity=:H1)

shfns_g = get_fe_basis(V)
phys_shfns_g = change_domain(shfns_g,PhysicalDomain())
dofs_f = get_fe_dof_basis(V)
dofs_f(phys_shfns_g)

Of course, it works running on the newest commit of this branch, where the only diff is that I apply the trick of expressing the Modal C0 shapefuns as a trivial linear combination.

Thanks for taking a look at this.

@fverdugo fverdugo self-requested a review April 27, 2022 14:00
Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking with @ericneiva, the linear combination trick looks good to me.
Performance issues can be fixed in a second round.

@ericneiva
Copy link
Member Author

@santiagobadia If you agree and there are no additional comments from your side, could you please accept the PR?

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2022

Codecov Report

Merging #777 (5a2a156) into master (0068a25) will decrease coverage by 0.04%.
The diff coverage is 85.27%.

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
- Coverage   88.28%   88.24%   -0.05%     
==========================================
  Files         153      156       +3     
  Lines       16929    17312     +383     
==========================================
+ Hits        14946    15277     +331     
- Misses       1983     2035      +52     
Impacted Files Coverage Δ
src/Exports.jl 0.00% <ø> (ø)
src/ReferenceFEs/LinearCombinationDofVectors.jl 62.50% <62.50%> (ø)
src/Polynomials/ModalC0Bases.jl 84.89% <84.89%> (ø)
src/ReferenceFEs/ModalC0RefFEs.jl 88.70% <88.70%> (ø)
src/Geometry/DiscreteModels.jl 95.76% <100.00%> (+0.09%) ⬆️
src/ReferenceFEs/ReferenceFEInterfaces.jl 68.49% <100.00%> (+1.11%) ⬆️
src/ReferenceFEs/SerendipityRefFEs.jl 94.11% <100.00%> (+5.43%) ⬆️
src/FESpaces/CLagrangianFESpaces.jl 99.46% <0.00%> (+0.53%) ⬆️
src/Fields/FieldArrays.jl 92.77% <0.00%> (+0.57%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@santiagobadia santiagobadia merged commit 5a1b8b5 into master May 4, 2022
@santiagobadia santiagobadia deleted the add_modalC0_machinery branch May 4, 2022 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants