Closed DanielDoehring closed 1 day ago
This 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.
NEWS.md
with its PR number.Created with :heart: by the Trixi.jl community.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 96.16%. Comparing base (
def208b
) to head (cdd1bc7
). Report is 4 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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?
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
So there are some allocations due to the reconstruction of the indices
vector.
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 https://github.com/trixi-framework/Trixi.jl/pull/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. From my side this is good to go, so you are kindly invited to review this (again). :)
Thanks!
@Arpit-Babbar Any suggestions from your side to improve this PR? Otherwise, I would like to proceed as you suggested above
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.
@Arpit-Babbar Are there further breaking changes that you need for https://github.com/trixi-framework/Trixi.jl/pull/1920?
@sloede How's your review going? Do you have a rough estimate when you will have some time for it?
As the boundary indices need to be specified as a
Tuple
and no longer simply aSymbol
orVector
ofSymbol
s, this is breaking.
Why again is this change needed and couldn't remain a Vector
of symbols?
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.
The reason why this is breaking is that the boundary symbols have now to be supplied as a NTuple
, not just a Symbol
or a Vector
of Symbol
s. Thus, even in the non-AMR case the user interface changes.
This fixes https://github.com/trixi-framework/Trixi.jl/issues/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.