fix silenced unit conversion bugs #2897

neutrinoceros · 2020-09-12T08:35:55Z

PR Summary

Another bug discovered in #2851
Opening as a draft to expose the bug in CI before I apply the fix.

update:
Apparently the main issue is that fake_random_ds has a very fragile api: the kwargs fields and units have default values but there is no mechanism to prevent using non-default fields without explicitly specifying their units. As a results we have a bunch of tests where we build a "temperature" field with units "cm/s".
These should now be fixed here, but I'm not fixing fake_random_ds's api as a whole for now (too much work for the time I have on my hands).

A different error is revealed in
yt/data_objects/tests/test_covering_grid.py::test_smoothed_covering_grid_2d_dataset, I'll try to investigate on this one.

neutrinoceros · 2020-09-12T10:54:58Z

So the error raised in
yt/data_objects/tests/test_covering_grid.py::test_smoothed_covering_grid_2d_dataset
is

    def test_smoothed_covering_grid_2d_dataset():
        ds = fake_random_ds([32, 32, 1], nprocs=4)
        ds.periodicity = (True, True, True)
        scg = ds.smoothed_covering_grid(1, [0.0, 0.0, 0.0], [32, 32, 1])
>       assert_equal(scg["density"].shape, [32, 32, 1])

yt/data_objects/tests/test_covering_grid.py:218:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
yt/data_objects/data_containers.py:268: in __getitem__
    f = self._determine_fields([key])[0]
yt/data_objects/data_containers.py:1473: in _determine_fields
    ftype, fname = self._tupleize_field(field)
yt/data_objects/data_containers.py:1443: in _tupleize_field
    finfo = self.ds._get_field_info(field)
yt/data_objects/static_output.py:808: in _get_field_info
    self.index
yt/data_objects/static_output.py:517: in index
    self.create_field_info()
yt/data_objects/static_output.py:601: in create_field_info
    self.field_info.load_all_plugins(self.default_fluid_type)
yt/fields/field_info_container.py:382: in load_all_plugins
    self.find_dependencies(loaded)
yt/fields/field_info_container.py:395: in find_dependencies
    deps, unavailable = self.check_derived_fields(loaded)
yt/fields/field_info_container.py:473: in check_derived_fields
    fd = fi.get_dependencies(ds=self.ds)
yt/fields/derived_field.py:247: in get_dependencies
    e[self.name]
yt/fields/field_detector.py:123: in __missing__
    vv = finfo(self)
yt/fields/derived_field.py:288: in __call__
    dd = self._function(self, data)
yt/fields/vector_operations.py:240: in _tangential
    data[ftype, f"{basename}_spherical_theta"] ** 2.0
yt/fields/field_detector.py:123: in __missing__
    vv = finfo(self)
yt/fields/derived_field.py:288: in __call__
    dd = self._function(self, data)
yt/fields/vector_operations.py:273: in _spherical_theta_component
    vectors = obtain_relative_velocity_vector(
yt/utilities/lib/misc_utilities.pyx:661: in yt.utilities.lib.misc_utilities.obtain_relative_velocity_vector
    vyg.convert_to_units(vxg.units)
/Users/clm/.pyenv/versions/3.8.5/envs/yt_dev/lib/python3.8/site-packages/unyt/array.py:617: in convert_to_units
    (conv_factor, offset) = self.units.get_conversion_factor(
/Users/clm/.pyenv/versions/3.8.5/envs/yt_dev/lib/python3.8/site-packages/unyt/unit_object.py:687: in get_conversion_factor
    return _get_conversion_factor(self, other_units, dtype)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

old_units = (dimensionless), new_units = cm/s, dtype = dtype('float64')

    def _get_conversion_factor(old_units, new_units, dtype):
        """
        Get the conversion factor between two units of equivalent dimensions. This
        is the number you multiply data by to convert from values in `old_units` to
        values in `new_units`.

        Parameters
        ----------
        old_units: str or Unit object
            The current units.
        new_units : str or Unit object
            The units we want.
        dtype: NumPy dtype
            The dtype of the conversion factor

        Returns
        -------
        conversion_factor : float
            `old_units / new_units`
        offset : float or None
            Offset between the old unit and new unit.

        """
        if old_units.dimensions != new_units.dimensions:
>           raise UnitConversionError(
                old_units, old_units.dimensions, new_units, new_units.dimensions
            )
E           unyt.exceptions.UnitConversionError: Cannot convert between 'dimensionless' (dim '1') and 'cm/s' (dim '(length)/(time)').

Which is happening for field == ('gas', 'velocity_spherical_theta') (and likely others down the road)
I think the root problem is that ('gas', 'velocity_spherical_theta') is meaningless for a 2D cartesian dataset.
I think the solution is to add a validator (possibly from a class that's not defined yet).

neutrinoceros · 2020-09-12T14:54:24Z

I was able to track the error down to

yt/yt/fields/vector_operations.py

Line 172 in 5fb1bea

# Is this safe?

So in 2D, we apparently replace a velocity field with ('index', 'zeros'), which is dimensionless, hence the error.
I guess the answer to this comment "is this safe ?" is "no, it's not :/"

neutrinoceros · 2020-09-12T15:50:02Z

ok so I think I'm headed in the right direction.
Right now on travis, excluding answer tests, we get only one error:

2642======================================================================
2643ERROR: yt.visualization.tests.test_fits_image.test_fits_image
2644----------------------------------------------------------------------
2645Traceback (most recent call last):
2646  File "/home/travis/build/yt-project/yt/venv/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
2647    self.test(*self.arg)
2648  File "/home/travis/build/yt-project/yt/yt/testing.py", line 803, in true_wrapper
2649    return func(*args, **kwargs)
2650  File "/home/travis/build/yt-project/yt/yt/visualization/tests/test_fits_image.py", line 64, in test_fits_image
2651    ds2.index
2652  File "/home/travis/build/yt-project/yt/yt/data_objects/static_output.py", line 517, in index
2653    self.create_field_info()
2654  File "/home/travis/build/yt-project/yt/yt/data_objects/static_output.py", line 601, in create_field_info
2655    self.field_info.load_all_plugins(self.default_fluid_type)
2656  File "/home/travis/build/yt-project/yt/yt/fields/field_info_container.py", line 382, in load_all_plugins
2657    self.find_dependencies(loaded)
2658  File "/home/travis/build/yt-project/yt/yt/fields/field_info_container.py", line 395, in find_dependencies
2659    deps, unavailable = self.check_derived_fields(loaded)
2660  File "/home/travis/build/yt-project/yt/yt/fields/field_info_container.py", line 473, in check_derived_fields
2661    fd = fi.get_dependencies(ds=self.ds)
2662  File "/home/travis/build/yt-project/yt/yt/fields/derived_field.py", line 247, in get_dependencies
2663    e[self.name]
2664  File "/home/travis/build/yt-project/yt/yt/fields/field_detector.py", line 123, in __missing__
2665    vv = finfo(self)
2666  File "/home/travis/build/yt-project/yt/yt/fields/derived_field.py", line 288, in __call__
2667    dd = self._function(self, data)
2668  File "/home/travis/build/yt-project/yt/yt/fields/astro_fields.py", line 121, in _entropy
2669    tr = data[ftype, "kT"] * data[ftype, "El_number_density"] ** mgammam1
2670  File "/home/travis/build/yt-project/yt/yt/fields/field_detector.py", line 123, in __missing__
2671    vv = finfo(self)
2672  File "/home/travis/build/yt-project/yt/yt/fields/derived_field.py", line 288, in __call__
2673    dd = self._function(self, data)
2674  File "/home/travis/build/yt-project/yt/yt/fields/fluid_fields.py", line 137, in _kT
2675    return (pc.kboltz * data[ftype, "temperature"]).in_units("keV")
2676  File "/home/travis/build/yt-project/yt/venv/lib/python3.6/site-packages/unyt/array.py", line 809, in in_units
2677    new_units, self.dtype
2678  File "/home/travis/build/yt-project/yt/venv/lib/python3.6/site-packages/unyt/unit_object.py", line 687, in get_conversion_factor
2679    return _get_conversion_factor(self, other_units, dtype)
2680  File "/home/travis/build/yt-project/yt/venv/lib/python3.6/site-packages/unyt/unit_object.py", line 933, in _get_conversion_factor
2681    old_units, old_units.dimensions, new_units, new_units.dimensions
2682unyt.exceptions.UnitConversionError: Cannot convert between 'cm**3*g/s**2' (dim '(length)**3*(mass)/(time)**2') and 'keV' (dim '(length)**2*(mass)/(time)**2').

Apparently this happens because FITSProjection has the side effect of transforming everything into a projected quantity (*= cm), without renaming fields, so we end up with a "temperature" field that's really represented in K*cm. I think the correct fix would be to call this a "projected_temperature" instead, even if this mean breaking some user code;

update: indeed the problem is more general than this. The projection object in FITSProjection is created by ds.proj, which I don't know the definition of.

update2: the units of projected fields are actually conditional, see

yt/yt/data_objects/construction_data_containers.py

Line 358 in dd2ec04

if self.weight_field is None:

so we only end up in this situation if there's no weight field.

matthewturk · 2020-09-12T20:08:33Z

yt/fields/field_info_container.py

@@ -470,6 +471,8 @@ def check_derived_fields(self, fields_to_check=None):
            fi = self[field]
            try:
                fd = fi.get_dependencies(ds=self.ds)
+            except UnitConversionError:


I have a suspicion that this is going to cause a lot of heartache. I do think it's the right thing to do.

One way we might make it an awful lot easier would be to make it much more obvious how to have the units deduced. As of right now, I don't think that's possible.

One way we might make it an awful lot easier would be to make it much more obvious how to have the units deduced. As of right now, I don't think that's possible.

at what level ? within the code base or from the user perspective ?

matthewturk · 2020-09-12T21:29:58Z

I meant from user perspective, when adding a derived field.

…

On Sat, Sep 12, 2020 at 3:51 PM Clément Robert ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In yt/fields/field_info_container.py <#2897 (comment)>: > @@ -470,6 +471,8 @@ def check_derived_fields(self, fields_to_check=None): fi = self[field] try: fd = fi.get_dependencies(ds=self.ds) + except UnitConversionError: One way we might make it an awful lot easier would be to make it much more obvious how to have the units deduced. As of right now, I don't think that's possible. at what level ? within the code base or from the user perspective ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2897 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO3MP4P5YB5LDXENRL3SFPNN3ANCNFSM4RJHORMA> .

yt/data_objects/tests/test_center_squeeze.py

matthewturk · 2020-09-12T21:32:13Z

usually density is mass density, so this is right

…

On Sat, Sep 12, 2020 at 4:31 PM Clément Robert ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In yt/data_objects/tests/test_center_squeeze.py <#2897 (comment)>: > @@ -6,7 +6,7 @@ def test_center_squeeze(): # create and test amr, random and particle data check_single_ds(fake_amr_ds(fields=("Density",))) - check_single_ds(fake_random_ds(16, fields=("Density",))) + check_single_ds(fake_random_ds(16, fields=("Density",), units=("g/cm**3",))) @matthewturk <https://github.com/matthewturk> I think I made a mistake here... "Density" is supposed to be a number density in "cm**(-3)", right ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2897 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO6WMRJVXF4MR5GAAMLSFPSDNANCNFSM4RJHORMA> .

neutrinoceros · 2020-09-12T22:03:09Z

Ok, I think this is back to a state where only answer tests on Travis, + maybe a handful of test on Jenkins should still fail.
Appeveyor pass this is a win, and I'll open for review when we're down to just answer tests miss.

neutrinoceros · 2020-09-13T16:11:44Z

About the remaining failures on Jenkins:
yt/frontends/boxlib/tests/test_outputs.py::test_CastroDataset loads a 2D Castro dataset and fails because somehow, particle_velocity_{ax} (with ax in "xyz") fields are dimensionless, while the bulk_velocity vector isn't, so there's a crash in the relative velocity calculation. Right now I really don't get how those fields end up being dimensionless.

matthewturk · 2020-11-25T15:57:02Z

I'm also not sure how they end up dimensionless without that just being another error that's propagating to where we're seeing it.

…-default fields

neutrinoceros · 2021-04-15T19:53:03Z

Just rebased to run tests again. There's a actually a non-zero chance that the remaining bugs magically went away because a lot of the issues I was trying to address here were solved on the main branch already (by @cphyc !). 🤞🏻

neutrinoceros · 2021-04-15T21:06:36Z

Nope, the dimensionless -> cm issue still here. I hope we can discuss this in a future triage

neutrinoceros · 2021-07-01T17:10:58Z

I think it's unlikely I'll ever manage to summon the will to fix this. Closing for now.

neutrinoceros added the bug label Sep 12, 2020

neutrinoceros force-pushed the hotfix_broken_unyt_conversion branch from ee205cc to 48a8e3b Compare September 12, 2020 14:02

neutrinoceros changed the title ~~fix a unit conversion bug~~ fix silenced unit conversion bugs Sep 12, 2020

neutrinoceros force-pushed the hotfix_broken_unyt_conversion branch 2 times, most recently from 0391e3a to eb96cc3 Compare September 12, 2020 17:51

matthewturk reviewed Sep 12, 2020

View reviewed changes

neutrinoceros force-pushed the hotfix_broken_unyt_conversion branch from 5b6fcd5 to d52efe8 Compare September 12, 2020 21:29

neutrinoceros commented Sep 12, 2020

View reviewed changes

yt/data_objects/tests/test_center_squeeze.py Outdated Show resolved Hide resolved

neutrinoceros force-pushed the hotfix_broken_unyt_conversion branch from d52efe8 to 3a01fcf Compare September 12, 2020 21:58

Base automatically changed from master to main January 20, 2021 15:27

neutrinoceros added 6 commits April 15, 2021 21:40

don't silently catch a UnitConversionError

2f9cb59

bugfix: explicitly pass units kwargs to fake_random_ds when using non…

42770c6

…-default fields

exp: remove a broken workaround (may unravel a lot of breakage)

6a045a0

hotfix: renounce the kT field for non-weighted projection data objects

ac9ea03

a safer api for fake_random_ds

ddb1dcd

fix: comply to new stricter api in fake_random_ds

e162507

neutrinoceros force-pushed the hotfix_broken_unyt_conversion branch from 3a01fcf to e162507 Compare April 15, 2021 19:51

remove unused import

e122e72

neutrinoceros closed this Jul 1, 2021

neutrinoceros deleted the hotfix_broken_unyt_conversion branch July 1, 2021 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix silenced unit conversion bugs #2897

fix silenced unit conversion bugs #2897

neutrinoceros commented Sep 12, 2020 •

edited

neutrinoceros commented Sep 12, 2020 •

edited

neutrinoceros commented Sep 12, 2020

neutrinoceros commented Sep 12, 2020 •

edited

matthewturk Sep 12, 2020

neutrinoceros Sep 12, 2020

matthewturk commented Sep 12, 2020 via email

matthewturk commented Sep 12, 2020 via email

neutrinoceros commented Sep 12, 2020

neutrinoceros commented Sep 13, 2020

matthewturk commented Nov 25, 2020

neutrinoceros commented Apr 15, 2021

neutrinoceros commented Apr 15, 2021

neutrinoceros commented Jul 1, 2021

fix silenced unit conversion bugs #2897

fix silenced unit conversion bugs #2897

Conversation

neutrinoceros commented Sep 12, 2020 • edited

PR Summary

neutrinoceros commented Sep 12, 2020 • edited

neutrinoceros commented Sep 12, 2020

neutrinoceros commented Sep 12, 2020 • edited

matthewturk Sep 12, 2020

Choose a reason for hiding this comment

neutrinoceros Sep 12, 2020

Choose a reason for hiding this comment

matthewturk commented Sep 12, 2020 via email

matthewturk commented Sep 12, 2020 via email

neutrinoceros commented Sep 12, 2020

neutrinoceros commented Sep 13, 2020

matthewturk commented Nov 25, 2020

neutrinoceros commented Apr 15, 2021

neutrinoceros commented Apr 15, 2021

neutrinoceros commented Jul 1, 2021

neutrinoceros commented Sep 12, 2020 •

edited

neutrinoceros commented Sep 12, 2020 •

edited

neutrinoceros commented Sep 12, 2020 •

edited