Skip to content

[Trivial] Correct documentation and usage of numlevel#1339

Open
adamdempsey90 wants to merge 2 commits intodevelopfrom
dempsey/numlevel
Open

[Trivial] Correct documentation and usage of numlevel#1339
adamdempsey90 wants to merge 2 commits intodevelopfrom
dempsey/numlevel

Conversation

@adamdempsey90
Copy link
Collaborator

PR Summary

numlevel is only read and used with AMR, but the docstring reads as if you need it with SMR. This has caused some confusion, so I'm correcting it.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@adamdempsey90
Copy link
Collaborator Author

@par-hermes format

@Yurlungur
Copy link
Collaborator

Thanks for the fix!

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

if (ref_lev > max_level_ref_) {
inside a
if (pib->block_name.compare(0, 27, "parthenon/static_refinement") == 0) {
conditional leads me to believe that we are using the max level info also for static mesh refinement.
I also think that this makes from a logical point of view in cases where I want to have some regions statically refined and other adaptively.
Overall, there should be a global max level (which is the current parameter), and then there's the level for the static regions individually as well as for the adaptive criteria.

@adamdempsey90
Copy link
Collaborator Author

inside a
if (pib->block_name.compare(0, 27, "parthenon/static_refinement") == 0) {
conditional leads me to believe that we are using the max level info also for static mesh refinement.

max_level_ref_ is hard coded to 63 when refinement == static. Also, if you do set numlevel for SMR, you'll get a warning that the parameter is never used.

I also think that this makes from a logical point of view in cases where I want to have some regions statically refined and other adaptively.

AMR + static_refinement blocks currently does not work (I would love for it to though).

@adamdempsey90
Copy link
Collaborator Author

inside a
if (pib->block_name.compare(0, 27, "parthenon/static_refinement") == 0) {
conditional leads me to believe that we are using the max level info also for static mesh refinement.

max_level_ref_ is hard coded to 63 when refinement == static. Also, if you do set numlevel for SMR, you'll get a warning that the parameter is never used.

I also think that this makes from a logical point of view in cases where I want to have some regions statically refined and other adaptively.

AMR + static_refinement blocks currently does not work (I would love for it to though).

This might just be an artemis thing though.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Thanks for pointing out the default value for static meshes.

At some point point, allowing both SMR and AMR would be nice.


<parthenon/mesh>
refinement = none
numlevel = 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

AMR is done for this test case (enabled via command line parameters for some steps).
So this needs to either stay or added to the test cases with AMR in advection_convergence.py

Comment on lines +215 to +217
max_level_ref_ =
pin->GetOrAddInteger("parthenon/mesh", "numlevel", 1,
"maximum level of refinement globally when AMR is on");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change might need a rebuilt of the autogenerated parameter docs as the string changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants