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

Read AMR domains lazily #4734

Merged
merged 38 commits into from
Dec 6, 2023
Merged

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Nov 6, 2023

PR Summary

For RAMSES, we already have some domain-based decomposition following a Hilbert curve. This leverages this to quickly find which domain to read when selecting a subregion, allowing to (costly) reading of many AMR domains.

New things

  • When using selection containers, only the domains that intersect the bounding box are now checked for intersection and their AMR structure read So if you're only reading 1% of the simulation, you do not need to read most of the AMR
  • When reading particles, we actually do not need to read the AMR structure. The benefit of the improvement above is that it allows us to completely ignore the AMR when reading only the particles. This is a MASSIVE speedup for DM-only simulations, or when focusing only on e.g. stars in a galaxy formation simulation.

This sits nicely on top of #4720 which allows much improved performances. I'd be happy to add some figures if required.

@cphyc cphyc added code frontends Things related to specific frontends refactor improve readability, maintainability, modularity enhancement Making something better and removed refactor improve readability, maintainability, modularity labels Nov 6, 2023
@cphyc
Copy link
Member Author

cphyc commented Nov 6, 2023

@yt-fido test this please

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

This looks good. Should more of the properties in the domain files be converted to cached_properties to reduce the levels of indirection? Like for instance, amr_header?

@@ -197,7 +204,7 @@ def __init__(self, ds, domain_id):
for t in ["grav", "amr"]:
setattr(self, f"{t}_fn", basename % t)
self._part_file_descriptor = part_file_descriptor
self._read_amr_header()
self.max_level = self.ds.parameters["levelmax"] - self.ds.parameters["levelmin"]
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I feel like I've seen this as an issue elsewhere. Are we correctly setting min_level as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to say I'm a bit confused about max_level/min_level. In RAMSES standard, these are simply related to the number of levels of refinement from the root, i.e. 1/2**level is the current cell size.
But for yt, if max_level is how more refined the deepest level is compared to the base grid, what does min_level min? Should it be 0 if there are some cells that are at the resolution of the base grid?

@cphyc
Copy link
Member Author

cphyc commented Nov 23, 2023

This looks good. Should more of the properties in the domain files be converted to cached_properties to reduce the levels of indirection? Like for instance, amr_header?

I am not sure what you exactly mean. Are you suggesting to add more things as cached_properties to make the dataset as lazy as possible, or the opposite to reduce indirections?

yt/frontends/ramses/data_structures.py Outdated Show resolved Hide resolved
yt/frontends/ramses/data_structures.py Show resolved Hide resolved
yt/frontends/ramses/data_structures.py Show resolved Hide resolved
yt/frontends/ramses/data_structures.py Show resolved Hide resolved
yt/frontends/ramses/hilbert.py Outdated Show resolved Hide resolved
yt/frontends/ramses/hilbert.py Show resolved Hide resolved
yt/frontends/ramses/io.py Outdated Show resolved Hide resolved
yt/frontends/ramses/particle_handlers.py Outdated Show resolved Hide resolved
yt/frontends/ramses/particle_handlers.py Outdated Show resolved Hide resolved
@cphyc
Copy link
Member Author

cphyc commented Nov 27, 2023

@yt-fido test this please.

neutrinoceros
neutrinoceros previously approved these changes Dec 6, 2023
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

All my points were addressed, thank you !

@neutrinoceros neutrinoceros added this to the 4.4.0 milestone Dec 6, 2023
@matthewturk matthewturk merged commit 1e90fd7 into yt-project:main Dec 6, 2023
13 checks passed
@cphyc cphyc deleted the RAMSES-lazy-loading branch April 9, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code frontends Things related to specific frontends enhancement Making something better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants