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

[BUG] Allow smoothing of derived SPH fields onto covering grids #4431

Merged
merged 8 commits into from
May 5, 2023

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented May 2, 2023

PR Summary

The following script fails on main:

from yt import load
bbox = [[40669.34, 56669.34], [45984.04, 61984.04], [54114.9, 70114.9]]
ds = load("TNGHalo/halo_59.hdf5", bounding_box=bbox)
w = ds.quan(0.2, "Mpc")
le = ds.domain_center-0.5*w
re = ds.domain_center+0.5*w
g = ds.r[le[0]:re[0]:128j,le[1]:re[1]:128j,le[2]:re[2]:128j]
print(g["gas","temperature"].min())
print(g["gas","kT"].min())

with this error:

  File "/Users/jzuhone/fix_grid.py", line 9, in <module>
    print(g["gas","kT"].min())
          ~^^^^^^^^^^^^
  File "/Users/jzuhone/Source/yt/yt/data_objects/data_containers.py", line 235, in __getitem__
    self.get_data(f)
  File "/Users/jzuhone/Source/yt/yt/data_objects/construction_data_containers.py", line 899, in get_data
    self._fill_sph_particles(fields)
  File "/Users/jzuhone/Source/yt/yt/data_objects/construction_data_containers.py", line 977, in _fill_sph_particles
    raise KeyError(f"{ptype} is not a SPH particle type!")
KeyError: 'gas is not a SPH particle type!'

It appears that derived fields were not supported for this operation. This PR addresses this issue by making extra checks to determine if a field is either an SPH on-disk field, derived field, or a particle filter type of an SPH field.

However, in order to avoid many more onerous code changes, I had to provide for the creation of fields such as ("gas", "particle_position"), ("gas", "particle_mass"), and ("gas","particle_position_[xyz]"), which currently do not exist because the SPH position fields are aliased to ("gas", "[xyz]") and the mass field is aliased to ("gas", "mass"). Happy to discuss if this doesn't feel right to anyone.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@jzuhone jzuhone added the bug label May 2, 2023
@jzuhone jzuhone changed the title Allow smoothing of derived SPH fields onto covering grids [BUG] Allow smoothing of derived SPH fields onto covering grids May 2, 2023
matthewturk
matthewturk previously approved these changes May 2, 2023
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.

So I am OK with this, and I think it's OK to go in as-is, but I did have two requests that might help future maintainers.

@@ -950,6 +950,28 @@ def _fill_particles(self, part):
for p in part:
self[p] = self._data_source[p]

def _check_sph_type(self, fi):
Copy link
Member

Choose a reason for hiding this comment

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

Can we change fi to finfo for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ptype = field[0]
else:
if fi.name[0] not in sph_ptypes and fi.name[0] != "gas":
raise_error = True
Copy link
Member

Choose a reason for hiding this comment

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

I know I should be able to figure this out, but the nested logic is not super easy to follow. Can we put in a comment a 'truth table' or something to show the cases we're evaluating here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jzuhone
Copy link
Contributor Author

jzuhone commented May 3, 2023

pre-commit.ci autofix

Co-authored-by: Clément Robert <cr52@protonmail.com>
@jzuhone
Copy link
Contributor Author

jzuhone commented May 4, 2023

I think this should be ready to go now.

@neutrinoceros neutrinoceros merged commit f0a62a3 into yt-project:main May 5, 2023
12 checks passed
@jzuhone jzuhone deleted the fix_sph_grid branch May 5, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants