-
Notifications
You must be signed in to change notification settings - Fork 112
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
Conservative AMR #2028
Conservative AMR #2028
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2028 +/- ##
==========================================
+ Coverage 96.27% 96.29% +0.02%
==========================================
Files 462 466 +4
Lines 37052 37226 +174
==========================================
+ Hits 35671 35845 +174
Misses 1381 1381
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks a lot Andrew! One thing it would suggest is that we add (possibly only in the test directory) a test for conservation. This way we can ensure this issue does not pop up later on again. |
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.
I have two suggestions for explanatory comments which I feel would be useful when added. The code formatter does cruel things, though.
Thanks! I can add comments to explain why the |
I think the (occasional) failing MPI test on |
….jl into conservative-amr
Yes, indeed. I could narrow it down to the interaction between t8code, MPI and Julia's garbage collector. I offer a workaround in #1980. It boils down to explicitly finalizing the # Finalize `T8codeMesh` to make sure MPI related objects in t8code are
# released before `MPI` finalizes.
!isinteractive() && finalize(mesh) I think it is best to push a PR just doing this. I'll do it right now. Then you can merge it in. Give me a minute. |
Just pushed this PR #2043. |
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.
Thanks a lot! I just have a last round of suggestions
Co-authored-by: Hendrik Ranocha <[email protected]>
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.
Thanks!
@andrewwinters5000 I added a NEWS.md entry since this is a kind of bigger change. Could you please check whether it looks good to you? |
Co-authored-by: Andrew Winters <[email protected]>
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.
Thanks
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.
Nice work - thanks a lot for investigating and fixing this, @andrewwinters5000
Now the Jacobian weighted solution
Ju
rather than only the solutionu
is interpolated and/or projected during the coarsen and refinement of the AMR procedure. This maintains conservation on 2D and 3D curvilinear meshes. Some tests may need updated now that the refine/coarsen procedures change.Update: My hopes were too optimistic. It seems many test values need updated :(