Issue 4174: Improvements to field_to_include specifications in create_firefly_object

[mtryan83]

PR Summary

Added code to allow fields that aren't in all particle types and/or are specified in (field_type,field_name) form.
Previously, fields_to_include could only consist of fields that were universal to all raw particle types. This was
not specified in the documentation and didn't appear to be "natural" behavior.
Lines 892-901 in yt/data_objects/data_containers.py incidentally fixes a bug when no fields are specified.

This is intended to fix #4174.

PR Checklist

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

[mtryan83]

pre-commit.ci autofix

[neutrinoceros]

In general it's hard to know for sure this is working as expected without tests, could you add some ?

[mtryan83]

In general it's hard to know for sure this is working as expected without tests, could you add some ?

I've added some basic tests (d5be708), but I'm not super familiar with this testing system. Is this what you're looking for?
I mainly focused just on the new field specifications, but did pick up another bug incidentally (see d3209c7), since there weren't any tests to start with.

[neutrinoceros]

Thank you, and nice catch ! I don't have enough time to do a detailed review right now, but here are some general key advice for writing tests:

• never call test functions explicitly
• if you need to repeat a test with a different parameter set, use @pytest.mark.parametrize
• if there are no firefly tests already, I think it'd be best to add a dedicated test file for these new ones
• if you need to reuse a dataset for different tests (useful if loading is expensive), use a @pytest.fixture. This can be a bit tricky so don't hesitate with any questions if you need, and this step is not necessary

[mtryan83]

Sorry, I just got back to working on this.

• never call test functions explicitly

This was a combination of a bad function name and function creep, my bad.

• if you need to repeat a test with a different parameter set, use @pytest.mark.parametrize

Done. This also helped with the previous point.

• if there are no firefly tests already, I think it'd be best to add a dedicated test file for these new ones

I'm just blind apparently. I moved the addition to the already existing test_firefly.py

• if you need to reuse a dataset for different tests (useful if loading is expensive), use a @pytest.fixture. This can be a bit tricky so don't hesitate with any questions if you need, and this step is not necessary

Done to simplify the @pytest.mark.parametrize stuff. I think I did this correctly, but I'd appreciate a double-check. I've never used some of these capabilities before.

Two general questions: am I trying to do too much with one test function? And is there an equivalent to pytest.warns for the mylog.warning function?

[neutrinoceros]

Thank you for keeping poking at this !

There are some functional issues with how you refactored your tests, as can be seen in
https://tests.yt-project.org/job/yt_py38_git/6076/testReport/junit/yt.data_objects.tests/test_firefly/firefly_test_dataset/
(though I'm not sure I actually understand went wrong here TBH; but let's work through it)

I want to add that, unfortunately we're still slowly migrating from nose to pytest, so some of our infrastructure using nose, and doesn't understand pytest. This means that any test file using pytest should be explicitly ignored in nose.
This requires updates to tests/tests.yaml (ass a --ignore-file entry), and nose_unit.cfg (update the ignore-files regex).

Now to answer your questions

am I trying to do too much with one test function?

probably yeah. We have a bunch of very busy tests functions already so I feel like I need to be tolerant about those, but it's best to avoid them if the tests can also be expressed as smaller, more numerous and decoupled functions.
AFAIC, the DRY (Don't Repeat Yourself) principle doesn't apply to tests, and trying to never repeat test code actually hurts readability as well as maintainability IMO.

And is there an equivalent to pytest.warns for the mylog.warning function?

Sort of. There's caplog, but it's pretty hard to use within yt tests because logging is typically disabled in CI:

yt/tests/ci_install.sh

Line 25 in eda90ff

echo "suppress_stream_logging = true" >> $HOME/.config/yt/yt.toml

There's a precedent for tests that still use it in yt/tests/test_load_sample.py but it's not as straightforward as I'd like. Don't feel like you have to go through this, we have very little logging tests, but of course feel free to if you'd like !

[mtryan83]

There are some functional issues with how you refactored your tests, as can be seen in https://tests.yt-project.org/job/yt_py38_git/6076/testReport/junit/yt.data_objects.tests/test_firefly/firefly_test_dataset/ (though I'm not sure I actually understand went wrong here TBH; but let's work through it)

I think I know what's wrong actually, I was passing the dataset and data region out of the fixture as a tuple, and then unpacking the tuple in the test function:

def test_fields_specification( firefly_test_dataset, ... ):
    (ds, dd) = firefly_test_dataset

because I only need dd, but if I don't keep ds it gets garbage collected and the whole thing breaks. But I guess pytest thinks I'm calling the fixture twice, instead of using the input parameter. The solution was to change the fixture to only return the dataset and just get the data region in the test function:
dd = firefly_test_dataset.all_data()
(I didn't see the fixture message when I ran on command line or I would have fixed it already)

I want to add that, unfortunately we're still slowly migrating from nose to pytest, so some of our infrastructure using nose, and doesn't understand pytest. This means that any test file using pytest should be explicitly ignored in nose. This requires updates to tests/tests.yaml (ass a --ignore-file entry), and nose_unit.cfg (update the ignore-files regex).

Updated.

probably yeah. We have a bunch of very busy tests functions already so I feel like I need to be tolerant about those, but it's best to avoid them if the tests can also be expressed as smaller, more numerous and decoupled functions. AFAIC, the DRY (Don't Repeat Yourself) principle doesn't apply to tests, and trying to never repeat test code actually hurts readability as well as maintainability IMO.

Hmm. I think I can split it into testing field names and field tuples, so I'll try that. That should get rid of some of the conditional logic and make it more obvious what's being tested.

And is there an equivalent to pytest.warns for the mylog.warning function?

Sort of. There's caplog, but it's pretty hard to use within yt tests because logging is typically disabled in CI:

yt/tests/ci_install.sh

Line 25 in eda90ff

echo "suppress_stream_logging = true" >> $HOME/.config/yt/yt.toml

There's a precedent for tests that still use it in yt/tests/test_load_sample.py but it's not as straightforward as I'd like. Don't feel like you have to go through this, we have very little logging tests, but of course feel free to if you'd like !

I was mainly thinking of using it to test the "detected but did not request"/"requested but not available" logic, but if logging is disabled typically anyway, there's not much use.

[mtryan83]

I split up the test function into 3: field strings, field tuples, and mixed strings and tuples.

I also noticed that the old firefly code tests (test_firefly_JSON_string and test_firefly_write_to_disk) were being skipped because they were referencing the old module name. I fixed that and updated the tests slightly to match the new firefly api (ae362e9), but they don't really look that useful. I marked the the test_firefly_write_to_disk test as skipped because I don't think it actually tests that anything is written correctly. Anything more would probably be better as a new PR or something. Feel free to revert the commit if you want.

[neutrinoceros]

but if logging is disabled typically anyway

... that's in the context of testing. I don't know what most users do with yt's logger, but by default it's always on and set to INFO level

[neutrinoceros]

Thank you again for your effort. I'll do a comprehensive review in the next few days !

[mtryan83]

... that's in the context of testing. I don't know what most users do with yt's logger, but by default it's always on and set to INFO level

Sorry, I just meant in the testing context. I'll definitely leave those messages in!

[neutrinoceros]

Here are important suggestions. I think if we agree on this I might go for one last, smaller review after this is settled

[mtryan83]

I'm having trouble with the test cases. I can't seem to get e.g. dd["Temperature"] to work, when I know it's a unique field (and works outside of running pytest) because it always falls back to searching for ("all", "Temperature"). Is there something that needs to be set in the test function to help with that?

[neutrinoceros]

(and works outside of running pytest)

Can you produce a minimal example for this ? I do not understand why it would fail only within pytest, so I'm tempted to think you're actually running slightly different code.

[mtryan83]

Can you produce a minimal example for this ? I do not understand why it would fail only within pytest, so I'm tempted to think you're actually running slightly different code.

You're absolutely right. I was using different datasets outside of pytest (e.g. the gizmo_mhd_mwdisk) but hadn't actually tried creating the fake_particle_ds outside of pytest. So the problem is with the fake_particle_ds and specifically when there are 2+ particle types (removing pt1 causes the ad["field1"] call to succeed both times). Here's an MWE:

import yt
from yt.utilities.exceptions import YTFieldNotFound
from yt.testing import fake_particle_ds
# create dataset
def test_fake_dataset():
    ds_fields = [  
        ("pt1", "particle_position_x"),
        ("pt1", "particle_position_y"),
        ("pt1", "particle_position_z"),
        ("pt2", "particle_position_x"),
        ("pt2", "particle_position_y"),
        ("pt2", "particle_position_z"),
        ("pt2", "field1"), 
    ]
    ds_field_units = ["code_length"]*7
    ds_negative = [0]*7
    ds = fake_particle_ds(
        fields=ds_fields,
        units=ds_field_units,
        negative=ds_negative,
    )
    return ds

ds = test_fake_dataset()
ad = ds.all_data()
try: 
    print(ad["field1"])
except(YTFieldNotFound):
    print("Expected error, looking for (all, field1) which DNE")
print(ad["pt2","field1"]) # Setting gas1 as most recently requested -> works elsewhere
try:
     print(ad["field1"])
except(YTFieldNotFound):
    print("ERROR: This should have produced the same result as (pt2, field1)")

Is there something I'm supposed to call when I'm creating the fake_particle_ds that will fix this?

[neutrinoceros]

I see that your expectations don't match the observed behaviour, but that behaviour is intended: calling ad["field1"] should yield the same error (if any) regardless of the query history. The intended rule is that an error should be raised whenever the user requests a field with an implicit field type that is not uniquely matched, and it seems to me that it's exactly what we're seeing in your example.

[mtryan83]

whenever the user requests a field with an implicit field type that is not uniquely matched,

I must be misunderstanding something then, isn't this the error case for ambiguous fields? I wouldn't have thought that would apply.
I was assuming that the implicit field type here is pt2, as that is the only field type with the field field1 (confirmed(?) using e.g. ds.fields), and so there should only be one unique match: (pt2, field1). Is that wrong? Is the implicit field type something else?

[neutrinoceros]

isn't this the error case for ambiguous fields?

yes, but ambiguity is not affected by access history !
Implicit field types are only ever accepted when there's exactly one match in the whole field collection. This is done precisely to avoid confusion and misconceptions; yt < 4.1 wasn't doing anything as smart as you seemed to assume, it simply took the first matching field and rolled with it, and ordering wasn't actually clearly defined, so it was all just undefined behaviour !

Btw, some of the test failures that you can see in you most recent run are completely unrelated to your PR. I'm assuming we can expect them to be resolved upstream in a matter of hours to days. See #4224 for context.

[mtryan83]

I think there's still a disconnect between what we're talking about, so I'll try to ask in a different way.
I have two assumptions based on the 4.1 doc:

1. There is a certain kind of field that can be specified using solely a string name, not a (ftype, fname) tuple, such that e.g. ds["field1"] is valid.
2. The kind of specification in the first assumption should be valid for the fields_to_include parameter of the create_firefly_object method. For example, create_firefly_object( ..., fields_to_include=["field1"], ...).

My question is then how should I construct a test dataset to test assumption 2? Of course, either one of those assumptions could be wrong, in which case I think I can figure out the path forward. I just can't seem to create a test dataset that works assuming they're both correct.

I've updated the test file; it should be more obvious what is being tested and how.

[neutrinoceros]

Sorry for the delay, had a busy week...
I tried to read the existing code with a fresh look, and found that the existing logic in create_firefly_object is very much at odds with the rest of yt's API, which is probably why we're having a hard time aligning our points of view.

It also seems to me that yt's code is a little bit contradictory, which of course doesn't help making things clear.
Specifically, the special particle type "all" could represent a set union or an intersection, the name itself isn't without ambiguity, but I guess that would be fine if we had one consistent convention. However, it is defined as a ParticleUnion object, but implemented as a set intersection. This contradiction is already apparent within a 5 lines interval here:

yt/yt/data_objects/static_output.py

Lines 728 to 733 in 45dc49b

	def add_particle_union(self, union):
	# No string lookups here, we need an actual union.
	f = self.particle_fields_by_type
	# find fields common to all particle types in the union
	fields = set_intersection([f[s] for s in union if s in self.particle_types_raw])

I am not sure what would be the best approach to fix this, but it seems that it could easily go way out of scope for the present PR.

Instead, I propose (contrary to what I previously said ! very sorry about that) that we keep the existing logic for create_firefly_object, and create actual unions of particle types. I think this is in line with the original design as well as what you had in mind initially; again I apologise for the confusion, that I think now was mainly on my side.
We could use a chunky comment explaining this in set theory terms within the function to reduce the risk that this confusion arises again in the future.

[mtryan83]

Instead, I propose (contrary to what I previously said ! very sorry about that) that we keep the existing logic for create_firefly_object, and create actual unions of particle types. I think this is in line with the original design as well as what you had in mind initially; again I apologise for the confusion, that I think now was mainly on my side. We could use a chunky comment explaining this in set theory terms within the function to reduce the risk that this confusion arises again in the future.

To make very sure I'm following correctly 😅 .

1. existing here means what's currently in e.g. yt 4.1, not the current state of this PR (and/or prior to 21beaf4)?

2. I'm not sure what exactly you mean by

   create actual unions of particle types

   Could you clarify, possibly with some psuedocode? Is this along the lines of the ("any", "field") construction I mentioned?

3. Would it be helpful to deprecate single string field specifications (and update the create_firefly_object doc to reflect this)? We'd still have to figure out something for now but we wouldn't need to worry about ambiguities/particle unions/etc in the future.

4. Following on 2. and 3., what about internally treating a single string spec ("field") as an ("any", "field") with a deprecation warning?

[neutrinoceros]

1. yes I mean yt 4.1
2. by "actual unions" I meant to describe yt 4.1's behaviour for this specific function: aggregate any particle types that match a given particle field, even if not all of them have it, so yes, that would be what we referred to as ("any", "field").
3. and 4. Since the current behaviour cannot be replicated with a different, existing API, maybe single string should still work, just not implicitly ? It could be kept an opt-in associated with a flag argument like match_any_particle_types, implemented like so

def create_firefly_object(
    ...,
    *,
    match_any_particle_types=None,
):
   if match_any_particle_types is None:
       # not specified, switching to (temporary) default
       issue_deprecation_warning(
           "match_any_particle_types wasn't specified. Its current default value (True) will be changed to False in the future. "
           "Pass an actual value (True or False) to silence this warning. ",
           ...
       )
       match_any_particle_types = True

WDYT ?

[neutrinoceros]

Now this has conflicts... I would solve them myself but you seem to be confortable with rebasing so ... can you do it (again) please ? Sorry this is turning so demanding, I will try to find another reviewer so hopefully we can put it at rest soon.

[matthewturk]

I'm happy to serve as a reviewer!

[matthewturk]

I went ahead and solved the conflicts

[chrishavlin]

This looks good to me, only some minor suggestions and questions.

[neutrinoceros]

@chrishavlin, feel free to merge if you feel it's ready. I'll be off for today

[chrishavlin]

@matthewturk did you still wanna take a look?