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/ENH: create_firefly_object only allow fields that are common to all particle types #4174

Closed
mtryan83 opened this issue Oct 18, 2022 · 4 comments · Fixed by #4175
Closed

Comments

@mtryan83
Copy link
Contributor

Bug report

Bug summary

The documentation of fields_to_include in the data_containers.create_firefly_object() method does not specify that only fields that are common to all particle types are allowed. This should either be mentioned in the documentation (Option 1) or the behavior changed to allow fields that are not common to all particle types (Option 2, preferred). (Open to other possibilities though)

Code for reproduction

# This uses the Gizmo gizmo_mhd_mwdisk sample dataset instead of the Ramses one in the
# "Exporting an Example Dataset to Firefly", but is otherwise mostly equivalent to that example.
# I've changed the names (ramses -> gizmo) and included the bounding box/unit base to 
# ensure the data is loaded correctly. This dataset includes six particle types: PartType0, which
# has the field "Temperature" and PartType1-5, which do not.
bbox = [[-600.0, 600.0], [-600.0, 600.0], [-600.0, 600.0]]
unit_base = {
    "length": (1.0, "kpc"),
    "velocity": (1.0, "km/s"),
    "mass": (1.0, "Msun"),
}
ge = yt.load("sample_data/gizmo_mhd_mwdisk/gizmo_mhd_mwdisk.hdf5",bounding_box=bbox,unit_base=unit_base)
region = ge.sphere(ge.domain_center, (1000, "kpc"))
reader = region.create_firefly_object(
       "GizmoExample",
       fields_to_include=["Temperature"],  # Temperature is only defined for PartType0
       fields_units=["code_temperature"],  
)

## adjust some of the options
reader.settings["color"]["io"] = [1, 1, 0, 1]  ## set default color
reader.particleGroups[0].decimation_factor = 100  ## increase the decimation factor

reader.writeToDisk()

Actual outcome

This produces the error:

YTFieldNotFound: Could not find field ('PartType1', 'Temperature') in gizmo_mhd_mwdisk.
Did you mean:
	('PartType0', 'Temperature')
	('PartType0', 'temperature')
	('gas', 'temperature')

And nothing is written to the disk.

Expected outcome

The expected behavior depends a bit on what option is chosen.

  • For the documentation change (Option 1), I guess I would expect a more useful error, perhaps:
YTFieldNotFound: Could not find field ('PartType1', 'Temperature') in gizmo_mhd_mwdisk.
Please include only fields that are common to all particle types.
Did you mean:
	('PartType0', 'Temperature')
	('PartType0', 'temperature')
	('gas', 'temperature')

with nothing written to the disk.

  • For Option 2, skipping the field if not present for that particle type, I would expect something like:
datadir: /XXXX/GizmoExample -- is not a sub-directory of firefly/static/data. 
This may produce confusing or inoperable results. As such, we will create a symlink for you when you writeToDisk.
Make sure each field_array (1) has a field_radius_flag (0), assuming False.
Make sure each field_array (1) has a field_radius_flag (0), assuming False.
Make sure each field_array (1) has a field_radius_flag (0), assuming False.
Make sure each field_array (1) has a field_radius_flag (0), assuming False.
Make sure each field_array (1) has a field_radius_flag (0), assuming False.
Make sure each field_array (1) has a field_radius_flag (0), assuming False.
Warning: Not all particle types contained field(s): Temperature                            <--- New
PartType0 - 4091/409013 particles - 3 tracked fields
PartType1 - 4375/437500 particles - 2 tracked fields
PartType2 - 2500/250000 particles - 2 tracked fields
PartType3 - 1250/125000 particles - 2 tracked fields
PartType4 - 766/76546 particles - 2 tracked fields
PartType5 - 1/1 particles - 2 tracked fields

And in Firefly, the ability to set Temperature as a selectable field.

Version Information

  • yt version: 4.1.1
  • Other Libraries (if applicable): firefly v 3.2.0

yt installed from source
python from anaconda
firefly from PyPI

@welcome
Copy link

welcome bot commented Oct 18, 2022

Hi, and welcome to yt! Thanks for opening your first issue. We have an issue template that helps us to gather relevant information to help diagnosing and fixing the issue.

@mtryan83
Copy link
Contributor Author

I'm happy to work on this, I just don't know which way would be preferred aka Option 1 vs 2 (vs 3?).

@neutrinoceros
Copy link
Member

I think fields that aren't available in every particle types should be allowed, as long as there's a clear warning about it.

@mtryan83
Copy link
Contributor Author

Thinking about it, it also made sense to allow field tuples like ("PartType1","Masses") to specify e.g. that masses should only be included for PartType, so I included that in #4175 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants