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

Grid refinement #838

Merged
merged 144 commits into from
Jan 26, 2023
Merged

Grid refinement #838

merged 144 commits into from
Jan 26, 2023

Conversation

JordiManyer
Copy link
Member

@JordiManyer JordiManyer commented Oct 17, 2022

First steps to implement grid adaptivity tools for Gridap:

  • RefinementRules and AdaptivityGlues, which encode the mapping between old and new cells in an adapted model.

  • Two new types AdaptedDiscreteModel and AdaptedDiscreteTriangulation, which represent a DiscreteModel and Triangulation that have been produced from a coarse model using some generic refinement method. They mostly wrap Gridap's equivalent types with some added features necessary to keep track of the adaptive hierarchy.

  • Tools to be able to transfer CellDatums back and forth between parent and child grids. These include changes to change_domain and a new type of Measure which allows the integration Triangulation to be different from the Triangulation of the resulting DomainContribution.

  • CompositeQuadratures, which allow to integrate over a Triangulation using RefinementRules in some or all cells (without needing a full AdaptedModel to be constructed.

@JordiManyer JordiManyer added the enhancement New feature or request label Oct 17, 2022
@JordiManyer JordiManyer self-assigned this Oct 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2022

Codecov Report

Merging #838 (b365f5a) into master (5491921) will increase coverage by 0.09%.
The diff coverage is 89.60%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #838      +/-   ##
==========================================
+ Coverage   88.33%   88.42%   +0.09%     
==========================================
  Files         164      172       +8     
  Lines       19178    19994     +816     
==========================================
+ Hits        16940    17679     +739     
- Misses       2238     2315      +77     
Impacted Files Coverage Δ
src/Geometry/Grids.jl 88.83% <0.00%> (+0.65%) ⬆️
src/ReferenceFEs/LagrangianRefFEs.jl 92.39% <0.00%> (-0.51%) ⬇️
src/ReferenceFEs/ReferenceFEInterfaces.jl 67.80% <0.00%> (-0.47%) ⬇️
src/CellData/CellDofs.jl 50.76% <28.57%> (-1.54%) ⬇️
src/CellData/CellFields.jl 91.26% <71.42%> (-0.23%) ⬇️
src/CellData/DomainContributions.jl 91.95% <82.75%> (-3.36%) ⬇️
src/Adaptivity/AdaptedTriangulations.jl 83.20% <83.20%> (ø)
src/Adaptivity/AdaptedDiscreteModels.jl 84.14% <84.14%> (ø)
src/Adaptivity/CompositeQuadratures.jl 86.84% <86.84%> (ø)
src/Adaptivity/FineToCoarseReferenceFEs.jl 89.61% <89.61%> (ø)
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amartinhuertas
Copy link
Member

amartinhuertas commented Oct 20, 2022

Hi @JordiManyer ! Thanks for your work!

The first concern I have got in regards to this PR has to do with the naming convention chosen.

If am not wrong, we plan to also leverage all this machinery no only for GMG preconditioning, but also to transfer information among a mesh and its adapted version whenever we are in an AMR (adaptive mesh refinement and coarsening) context. Agreed?

In such a case the prefix Refined attached to all type names, files, etc. would not be 100% accurate. Perhaps, in order to be more accurate, we could use Adapted instead of Refined.

what do you think? @JordiManyer @santiagobadia ?

@santiagobadia
Copy link
Member

I think that the current implementation is for a coarse to fine transfer (and vice versa). Anyway, we can call it Adapted even though in this case it is Adapted by Refinement only. And if we just eliminate Refinement? (not sure it makes sense, I haven't checked the code)

@amartinhuertas
Copy link
Member

I think that the current implementation is for a coarse to fine transfer (and vice versa).

Agreed.

And if we just eliminate Refinement?

We cannot do that, e.g., RefinedDiscreteModel would become DiscreteModel.

@JordiManyer
Copy link
Member Author

JordiManyer commented Jan 23, 2023

As a note: I've solved most of the issues we had remaining, noticeably the orientation of the adapted meshes. However this has aggravated the only issue we have left, i.e the change_domain issue.

Let me explain: Since we now properly return the orientability state of the adapted mesh, in some cases we obtain non-oriented meshes from oriented ones. Noticeably, if we start from a mesh of QUADs and refine only some of the cells, we might obtain a mixed mesh (QUADs and TRIs). Right now, I cannot guarantee that the new mesh is oriented, so I return a non-oriented mesh.
However, this makes typeof(parent) !== typeof(adapted_model.model) and therefore is_change_possible returns false, which breaks the code. This is why the new tests are failing.

In order to close this PR once and for all, I see two way outs:
1 - Find a fix for our change_domain issue.
2 - Make the tests pass using a non-pathological case and document the issue for it to be solved in the future.

@amartinhuertas
Copy link
Member

Hi @JordiManyer ... thanks a lot for your work.

I would pursue to find a reasonable strategy for:

1 - Find a fix for our change_domain issue.

On the other hand, we have had a very long conversation above, with a lot of different branches, reasonings, etc.

Do you think it may make sense to gather all the pending tasks in a central point (say a new issue with tasks + pointers).

Otherwise, it might be easy to forget all the issues that might be pending?

@JordiManyer
Copy link
Member Author

JordiManyer commented Jan 23, 2023

Hi @amartinhuertas I agree this is getting very cluttered. Also, it is holding up things in other repos, which cannot be merged until this is dealt with. I have given it some though, and I think I've come up with the best solution:

In practice, we are only supporting the change of grid for BodyFittedTriangulations. Not only our current refine implementations do not return a facet-aware glue for all facet dimensions, the fine-to-coarse routines have only been tested for cells, i.e Dc==Dp.
Also, after some though I've come to the conclusion that the proper way of dealing with the change_domain issue probably involves creating some machinery to couple the triangulation FaceToFaceGlue (that all trians have and that is involved in the change_domain for normal triangulations) with our AdaptivityGlue.
This involves a lot of development, which I think can be bundled in a completely different branch.

So here is my way forward: I've limited change_domain to Adapted trians that have the same background model (where standard Gridap takes the reigns) and changes between BodyFittedTriangulations (which we can deal with and have been properly stressed out in our tests/other repos).
I propose we merge this branch now, and start implementing in another PR what we need for more complicated cases. I think I have an idea for where to start with this new PR, and we could take our time without this holding off everything else.
What do you think?

@amartinhuertas
Copy link
Member

What do you think?

Ok. Let us merge the current PR (with the fixes required to have tests passing, and properly @nonimplemented macros as per required) and start "fresh" with a new one that gathers all the tasks which are pending from the current one.

@amartinhuertas
Copy link
Member

Ok. Let us merge the current PR (with the fixes required to have tests passing, and properly @nonimplemented macros as per required) and start "fresh" with a new one that gathers all the tasks which are pending from the current one.

Please let me know when you believe the PR is ready to merge.

@JordiManyer
Copy link
Member Author

@amartinhuertas When the tests are done running, I believe this can be merged.

Copy link
Member

@amartinhuertas amartinhuertas left a comment

Choose a reason for hiding this comment

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

Ok. Let us accept this PR.

Then, let us create a brand new one that gathers in the PR description as bullet points all the devs and discussions we have had that are still to be resolved.

@amartinhuertas amartinhuertas merged commit 0c90ddc into master Jan 26, 2023
@JordiManyer JordiManyer deleted the refined-discrete-models branch January 27, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants