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

REG: recent regression in field unit detection #3959

Closed
neutrinoceros opened this issue Jun 7, 2022 · 3 comments · Fixed by #3962
Closed

REG: recent regression in field unit detection #3959

neutrinoceros opened this issue Jun 7, 2022 · 3 comments · Fixed by #3962
Labels
bug release critical Highest priority (in a milestone)
Milestone

Comments

@neutrinoceros
Copy link
Member

Bug report

Bug summary

Found a regression on the main branch that was discovered via a test added in #3849

this problem bisects to 60e9414 (#3921)

Code for reproduction

Here's a minimal reproducer, using dimensionless test arrays (noise1 and noise3)

from yt.testing import add_noise_fields, fake_random_ds
from yt.visualization.api import PhasePlot

ds = fake_random_ds(16)
add_noise_fields(ds)

my_sphere = ds.sphere("c", 1)
p = PhasePlot(
    my_sphere,
    ("gas", "noise1"),
    ("gas", "noise3"),
    [("gas", "density")],
)

Actual outcome

yt : [INFO     ] 2022-06-07 19:15:37,464 Parameters: current_time              = 0.0
yt : [INFO     ] 2022-06-07 19:15:37,464 Parameters: domain_dimensions         = [16 16 16]
yt : [INFO     ] 2022-06-07 19:15:37,465 Parameters: domain_left_edge          = [0. 0. 0.]
yt : [INFO     ] 2022-06-07 19:15:37,465 Parameters: domain_right_edge         = [1. 1. 1.]
yt : [INFO     ] 2022-06-07 19:15:37,466 Parameters: cosmological_simulation   = 0
yt : [WARNING  ] 2022-06-07 19:15:37,909 Field ('gas', 'noise1') was added without specifying units or dimensions, auto setting units to 
yt : [WARNING  ] 2022-06-07 19:15:37,909 Field ('gas', 'noise3') was added without specifying units or dimensions, auto setting units to 
Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t_reg_phase.py", line 8, in <module>
    p = PhasePlot(
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/profile_plotter.py", line 971, in __init__
    profile = create_profile(
  File "/Users/robcleme/dev/yt-project/yt/yt/data_objects/profiles.py", line 1454, in create_profile
    obj = cls(*args, **kwargs)
  File "/Users/robcleme/dev/yt-project/yt/yt/data_objects/profiles.py", line 733, in __init__
    x_min, x_max = _sanitize_min_max_units(
  File "/Users/robcleme/dev/yt-project/yt/yt/data_objects/profiles.py", line 35, in _sanitize_min_max_units
    rmin = amin.in_units(finfo.output_units)
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.10/site-packages/unyt/array.py", line 821, in in_units
    units = _sanitize_units_convert(units, self.units.registry)
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.10/site-packages/unyt/array.py", line 259, in _sanitize_units_convert
    unit = Unit(possible_units, registry=registry)
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.10/site-packages/unyt/unit_object.py", line 227, in __new__
    raise UnitParseError(
unyt.exceptions.UnitParseError: Unit representation must be a string or sympy Expr. 'None' has type '<class 'NoneType'>'.
@neutrinoceros neutrinoceros added bug release critical Highest priority (in a milestone) labels Jun 7, 2022
@neutrinoceros neutrinoceros added this to the 4.1.0 milestone Jun 7, 2022
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Jun 7, 2022

So I see two reasons why this is failing:

I think this is a grave oversight from #3921, and the best course of action right now is to revert it.

@neutrinoceros
Copy link
Member Author

at initialisation, profiles retrieve units from data_source.ds.field_info before calling YTSelectionContainer._generate_fields, so it doesn't work with #3921

After more careful checking, this turns out to be wrong, hence invalidating my previous conclusion

@neutrinoceros
Copy link
Member Author

I think I found the proper fix in #3962

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug release critical Highest priority (in a milestone)
Projects
None yet
1 participant