[feature] Simpler way to add derived fields#5208
[feature] Simpler way to add derived fields#5208cphyc wants to merge 16 commits intoyt-project:mainfrom
Conversation
|
I genuinely love this and I find it to be an incredibly nice, ergonomic way to add fields. |
|
I still think this is great! Sorry for the noise, but I am demonstrating commenting from the command line. |
|
Quick update: we can now do: import yt
ds = yt.load("output_00080")
# Quickly define new fields
ds.fields.gas.double_density = ds.fields.gas.density * 2
# Quickly filter on the grid
ad = ds.all_data()
left = ad[ds.fields.gas.x < (0.5, "unitary")]
# Quickly filter particles
left_particles = ds.fields.io.particle_position_x < ds.quan(0.5, "unitary")
ds.add_particle_filter(left_particles)
ad[left_particles.name, "particle_position_x"] |
|
Note to reviewers: I'm waiting to get approval (if any :D) before writing the documentation. If you'd like to see more examples, I can write the doc before getting approval though. |
yt/fields/derived_field.py
Outdated
| def __gt__(self, other) -> "DerivedFieldCombination": | ||
| return DerivedFieldCombination([self, other], op=operator.gt) | ||
|
|
||
| # def __eq__(self, other) -> "DerivedFieldCombination": |
There was a problem hiding this comment.
Could you leave a comment as to why this is commented out?
There was a problem hiding this comment.
Ah good point! I went ahead and implemented this feature.
The annoying point is that defining an __eq__ method forces you to also implement a __hash__ method (which I did).
But this is worth it, since now we can write something like
ad = ds.all_data()
ds.fields.index.is_level_0 = ds.fields.index.grid_level == 0
ad["index", "is_level_0"]
low_res_only = ad.cut_region(ds.fields.index.grid_level == 0)b7b59d9 to
0ba92ab
Compare
|
For what it's worth at this point, I like the new method! I did check out the branch and quickly played around with defining fields and it all worked as expected but I haven't had time to actually review the code (and probably won't in the short term...). So +1 conceptual approval from me, and don't hold this up on my behalf if anyone else has time to get to it. P.S., The most recent test failure does look related to the most recent changes. |
|
I found the source of the failures @chrishavlin was talking about. Here's the log of my little adventure:
# The following two evaluate to the same boolean
bool(ds.fields.ftype1.fname1 == ds.fields.ftype2.fname2)
ds.fields.ftype1.fname1 is ds.fields.ftype2.fname2 |
This is clunky, but this works ds = yt.load(output_00080) my_filter = ds.fields.io.particle_position_x < ds.quan(0.5, unitary) ds.add_particle_filter(my_filter) print(np.max(ds.r[my_filter.name, particle_position_x]).to(unitary))
This implements bool(field1 == field2), so that it returns True iff field1 is field2. For all other operators, it returns True (since the truthness of an object is True). It raises an error if we have more than two terms to the equality.
Mypy expects them to return a boolean. We return instead a combination of fields which evaluate to a boolean (through __bool__).
75ce14b to
265f445
Compare
|
@matthewturk @jzuhone @chrishavlin this should be ready to go now, tests (if they pass) + documentation included. |
PR Summary
This adds a simple syntax to define a new derived field.
This allows to write:
Open for discussion before I write the documentation (or we kill that PR :) )
PR Checklist