-
Notifications
You must be signed in to change notification settings - Fork 105
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
AMR-Compat SurfaceIntegrals #1959
AMR-Compat SurfaceIntegrals #1959
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. |
….jl into SurfaceIntegralsAMR
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1959 +/- ##
=======================================
Coverage 96.16% 96.16%
=======================================
Files 460 460
Lines 36980 36984 +4
=======================================
+ Hits 35560 35564 +4
Misses 1420 1420
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 for tackling this. How urgent is the fix for you? I'm asking since this is a hotfix with a breaking change. An alternative would be to implement a notification by the AMR callback as mentioned in #215. Thus, we may discuss whether this could be a more general approach that helps us in the long run
I plan to show/use this for the JuliaCon talk (this is also where I realized that this is buggy). I absolutely agree that #215 makes a hell lot of sense, but this also seems to me like a very-long range project. Until then, I suggest that we get the callback working also with AMR. |
Ok. How much overhead do we get by this for non-AMR cases? |
I added some timings: analyze solution 27 175ms 1.2% 6.48ms 564KiB 5.9% 20.9KiB
~analyze solution~ 27 174ms 1.2% 6.44ms 361KiB 3.8% 13.4KiB
AnalysisSurfaceIntegral 108 998μs 0.0% 9.24μs 203KiB 2.1% 1.88KiB
~AnalysisSurfaceIntegral~ 108 851μs 0.0% 7.88μs 752B 0.0% 6.96B
inidices 108 147μs 0.0% 1.36μs 202KiB 2.1% 1.88KiB for this elixir: https://github.com/trixi-framework/Trixi.jl/blob/main/examples/p4est_2d_dgsem/elixir_navierstokes_NACA0012airfoil_mach08.jl Note that the callback gets in this case called every 10th timestep, much more than in an actual case. But this allowed me to reduce the total runtime and the analysis interval. |
I see that this PR is mentioned in v0.8. Does it mean that it will be merged eventually? There is no urgency from my side, but it will be good to know. If this PR is going to be merged, we can make this nearly final (i.e., ready to merge whenever v0.8 is released) and rebase #1920 on top of this. |
Yes, I think it would be good to merge this somewhat soon and release v0.8. What's your take on this, @DanielDoehring? |
I added this to v0.8 as this is a breaking change, which we typically handle by increasing the first version decimal. |
Thanks! @Arpit-Babbar Any suggestions from your side to improve this PR? Otherwise, I would like to proceed as you suggested above
|
@Arpit-Babbar Are there further breaking changes that you need for #1920? |
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.
Everything looks good to me. The PR #1920 will not require any breaking changes.
@sloede How's your review going? Do you have a rough estimate when you will have some time for it? |
Why again is this change needed and couldn't remain a |
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.
Just some minor changes, but otherwise this LGTM.
I wonder, however, whether this is really a breaking change. From what I can see, this is really a bugfix, since the surface integral computation was already used for AMR simulations but just produced the wrong output. I thus think this shouldn't require a minor version bump, but only a patch release.
Having said that, if the majority thinks this requires v0.8, I'd not be opposed to it either.
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
The reason why this is breaking is that the boundary symbols have now to be supplied as a |
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!
This fixes #1955 by re-computing the boundary indices every time the surface integral is computed.
As the boundary indices need to be specified as a
Tuple
and no longer simply aSymbol
orVector
ofSymbol
s, this is breaking.