Skip to content
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

[ENH] RAMSES: set max_level from parameter file #5060

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Nov 20, 2024

PR Summary

RAMSES contains a text file that describes some basic element of the AMR structure, including the maximum level of refinment. However, that level may not be reached. Up to this PR, we would read all AMR domain files to figure out to effective max level of refinment. This is however a costly operation for large simulations; this PR returns a more conservative number for the max level (the highest achievable by the sim) and is MUCH faster.

PR Checklist

  • New features are documented, with docstrings and narrative docs -> no new feature
  • Adds a test for any bugs fixed. Adds tests for new features. -> all existing tests should pass

@max_level.setter
def max_level(self, value):
self._max_level = value
return self.ds.max_level
Copy link
Member Author

Choose a reason for hiding this comment

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

@neutrinoceros neutrinoceros added this to the 4.5.0 milestone Nov 25, 2024
This allows to, e.g. create a sphere object without ever reading most of the AMR file
@matthewturk
Copy link
Member

I'm not 100% on this. In my mind, the idea of the max_level parameter should tell the actual-thing-it-is, and the access of the parameter should give what the parameter is set to.

@cphyc
Copy link
Member Author

cphyc commented Dec 3, 2024

I'm not sure I understand what you mean.

To clarify, before this PR, max_level would be the actual level reached anywhere in the simulation. This PR replaces it with the maximum level that can (but may not) be reached anywhere in the simulation.

The motivation is that the former definition requires reading the entirety of the AMR structure of the simulation, while the latter only requires parsing one small text file.

As far as I could tell, the only place where max_level was being used was to make sure that, when selecting a sphere, the radius is at least one dx wide, with dx being the best resolution. Note that this doesn't prevent a sphere from having 0 cells (for example, if it is centred around a low-resolution region), so I changed the definition of max_level. If anything, it makes the test a bit more relaxed.

@matthewturk
Copy link
Member

Oh, I think I understand -- and I can see the value in speeding it up! What I'm saying is that the usage of max_level in Enzo is the max level in the index, whereas MaximumRefinementLevel (also in Enzo) is how you get the maximum allowed. Just that the property was actual, the parameter was allowed.

But maybe this is OK for RAMSES -- or maybe for the specific case of spheres, we don't do the check?

@cphyc
Copy link
Member Author

cphyc commented Dec 3, 2024

I see. I guess the issue is that, although max_level is defined for all datasets afaik, MaximumRefinementLevel is Enzo-specific. There is a similar parameter for RAMSES, but it has a different name (it's levelmax).

I can explore the idea of replacing occurrences of max_level with max_potential_level or some other jargon signalling it is an upper boundary to see what it'd take to have this and will set this PR in draft mode in the meantime.

@cphyc cphyc marked this pull request as draft December 3, 2024 21:36
@matthewturk
Copy link
Member

Is it possible to do an extremely quick pass to get the max level, without parsing the full file? I'm not able to convince myself.

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

Successfully merging this pull request may close these issues.

3 participants