-
Notifications
You must be signed in to change notification settings - Fork 213
Add adaptive horizon finder criteria #6689
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 adaptive horizon finder criteria #6689
Conversation
|
The clang-tidy failure did not come up when I ran the test locally, but I'll make the fix |
f635d66 to
d4a4fd3
Compare
knelli2
left a comment
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.
Looks good! Please add a fixup commit for these changes, and also fix the small conflict.
src/ParallelAlgorithms/ApparentHorizonFinder/Criteria/Criterion.hpp
Outdated
Show resolved
Hide resolved
| namespace ah::Criteria { | ||
| using standard_criteria = tmpl::list<IncreaseResolution>; |
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.
Do we want to allow this as part of the "standard criteria"? This means it'll probably be available to users in the input file if this type alias is used in the metavars. I know this is what AMR does, but seems like this is just a testing criterion.
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'm fine not including this in the standard criteria list. If users really want it for some reason we can always add it later
src/ParallelAlgorithms/ApparentHorizonFinder/Criteria/Factory.hpp
Outdated
Show resolved
Hide resolved
tests/Unit/ParallelAlgorithms/ApparentHorizonFinder/Criteria/Test_Criterion.cpp
Outdated
Show resolved
Hide resolved
d4a4fd3 to
88150d0
Compare
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.
You can squash everything
src/ParallelAlgorithms/ApparentHorizonFinder/Criteria/Criterion.hpp
Outdated
Show resolved
Hide resolved
88150d0 to
ba3371d
Compare
|
Got approval from @nilsvu on slack that I can just merge this without his review in the interest of time. Also test failure is unrelated |
Proposed changes
Adds a base class and a simple example derived class for criteria for adjusting the resolution adaptively when finding an apparent horizon. A future PR will implement the actual criteria intended to be used in real binary-black-hole simulations. Another future PR (or set or PRs) will take care of implementing adaptivity in the apparent horizon finder.
The design and implementation are based on how adaptive mesh refinement criteria are implemented in spectre but enables the criteria to work with output of a horizon find (Strahlkorper and FastFlow::IterInfo) instead of an ElementId.
NOTE: I used Cursor generative AI (tab completion, review, fixing compiler errors) when preparing this PR. I examined all code suggestions, and to the best of my knowledge, this PR contains no code either copyrighted by someone else or used in violation of any code's license agreement. Please note in your review any concerns about this.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments