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

Add three new derived fields to the AREPO frontend #3815

Merged
merged 7 commits into from
Feb 24, 2022

Conversation

jzuhone
Copy link
Contributor

@jzuhone jzuhone commented Feb 17, 2022

PR Summary

PR Checklist

  • Alias the ("PartType0","pressure") field to ("gas","pressure") (not sure why this wasn't already one
  • Create a "cell_volume" field
  • Create a "cosmic_ray_pressure" field
  • Rename "cr_energy_density" and "specific_cr_energy" to "cosmic_ray_energy_density" and "specific_cosmic_ray_energy"
  • 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 code frontends Things related to specific frontends label Feb 17, 2022
# Since the AREPO gas "particles" are Voronoi cells, we can
# define a volume here
def _volume(field, data):
return data[ptype, "mass"] / data[ptype, "density"]
Copy link
Member

Choose a reason for hiding this comment

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

Both mass and density are in the file, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right.

yt/frontends/arepo/fields.py Outdated Show resolved Hide resolved
@neutrinoceros neutrinoceros added the enhancement Making something better label Feb 17, 2022
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.

I'm not opposing that the new field be named "cr_pressure" but could it be aliased to a more explicit name "cosmic_ray_pressure" ?

@jzuhone
Copy link
Contributor Author

jzuhone commented Feb 22, 2022

@neutrinoceros if we're going to do that, we should probably also change specific_cr_energy and cr_energy_density also for consistency.

@neutrinoceros
Copy link
Member

Oh yes, I didn't notice these existed already. I would be in favour of adding aliases for them too indeed.

Comment on lines +32 to +37
"ArepoCosmicRays.tar.gz": {
"hash": "e664ea7f634fff778511e9ae7b2e375ba9c5099e427f7c037d48bba9944388f4",
"load_kwargs": {},
"load_name": "snapshot_039.hdf5",
"url": "https://yt-project.org/data/ArepoCosmicRays.tar.gz"
},
Copy link
Member

Choose a reason for hiding this comment

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

I've checked that this new dataset loads perfectly with yt.load_sample

Copy link
Member

@chummels chummels left a comment

Choose a reason for hiding this comment

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

I've looked this over, and everything looks good. Great to have CR support for AREPO as well as cell_volume and gas pressure fields for AREPO datasets.

@cphyc cphyc merged commit e7b5e3e into yt-project:main Feb 24, 2022
@jzuhone jzuhone deleted the more_arepo_fields branch June 21, 2022 17:21
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.

5 participants