Unyt #2219

ngoldbaum · 2019-03-26T19:54:21Z

Make yt use unyt.

WIP for now until I get the tests to pass.

…ns anymore

… registry

… tests

matthewturk

Wow! This is a ton of work, and lots of awesome changes. Thank you for doing this, @ngoldbaum . I've left some comments (mostly questions) inline, but I'm quite happy to see this effort.

matthewturk · 2019-04-03T18:22:44Z

doc/source/yt4differences.rst

+object has a large number of attributes corresponding to the names of units and
+physical constants. All units known to the dataset will be available, including
+custom units. In situations where you might have used ``ds.arr`` or ``ds.quan``
+before, you can now safely use ``ds.units``:


yt/data_objects/construction_data_containers.py

matthewturk · 2019-04-03T18:31:56Z

yt/data_objects/data_containers.py

@@ -643,7 +653,7 @@ def save_as_dataset(self, filename=None, fields=None):

        extra_attrs = dict([(arg, getattr(self, arg, None))
                            for arg in self._con_args + self._tds_attrs])
-        extra_attrs["con_args"] = self._con_args
+        extra_attrs["con_args"] = repr(self._con_args)


I'm not sure I understand why we need to do this -- _con_args is a tuple of strings, so it should serialize correctly in h5py without this. Now we're turning the tuple into a string that represents the tuple, yes?

h5py serializes a tuple of strings as a string array, this was leading to a case where I wasn't able to round-trip con_args anymore. Using repr and eval seemed like the most straightforward way to take care of that.

matthewturk · 2019-04-03T18:33:44Z

yt/data_objects/octree_subset.py

@@ -186,8 +186,8 @@ def deposit(self, positions, fields = None, method = None,
        op.initialize()
        mylog.debug("Depositing %s (%s^3) particles into %s Octs",
            positions.shape[0], positions.shape[0]**0.3333333, nvals[-1])
-        pos = np.asarray(positions.convert_to_units("code_length"),
-                         dtype="float64")
+        positions.convert_to_units("code_length")


I think as long as positions is an internally-generated field, it will be safe to drop the float64. But I did have to stop and think about that.

OK, I can add an astype('float64')

No no, I think astype will make a copy, and I think we're safe without it.

matthewturk · 2019-04-03T18:39:51Z

yt/data_objects/static_output.py

@@ -216,6 +217,70 @@ def __get__(self, instance, owner):
    def __set__(self, instance, value):
        self.data[instance] = value

+class ConstantContainer(object):


Can these go into a separate file? And I think for my own understanding, just a comment about what each is supposed to do (doesn't have to be docstrings or anything extensive) would be helpful.

yt/data_objects/tests/test_dataset_access.py

matthewturk · 2019-04-03T18:49:52Z

yt/fields/magnetic_field.py

@@ -62,7 +64,7 @@ def _magnetic_field_strength(field, data):

    def _magnetic_energy(field, data):
        B = data[ftype,"magnetic_field_strength"]
-        return 0.5*B*B/mag_factors[B.units.dimensions]
+        return 0.5*B*B/mag_factors(B.units.dimensions)


I sometimes get confused by closures -- since mag_factors only has ds from the outer namespace, I think it might be safer to make it accept ds as an argument and put it at the outermost function level.

matthewturk · 2019-04-03T18:53:03Z

yt/frontends/ytdata/data_structures.py

@@ -86,7 +86,7 @@ def _parse_parameter_file(self):
            for key in f.attrs.keys():
                v = parse_h5_attr(f, key)
                if key == "con_args":
-                    v = v.astype("str")
+                    v = eval(v)


This is the pair to the repr() call above, I think. I'm not totally sure this is safe. What's the use case for this particular set of changes?

I explained above in the question about repr. Not sure what you mean by "safe" - are you talking about a security concern or something? This is all data that's internal to yt...

brittonsmith · 2019-04-03T18:50:27Z

yt/data_objects/static_output.py

@@ -1068,7 +1147,7 @@ def set_units(self):
                              omega_lambda=self.omega_lambda,
                              omega_radiation=self.omega_radiation,
                              use_dark_factor = use_dark_factor,
-                              w_0 = w_0, w_a = w_a)
+                              w_0 = w_0, w_a = w_a, unit_registry=self.unit_registry)


This will likely have unintended consequences. Most of the cosmology calculations return values in the comoving frame but over multiple redshifts. The only way to make this possible was to equate the comoving and proper reference frames in the cosmology calculator's unit registry. I think the dataset and its cosmology calculator need to have distinct unit registries.

OK - can you give me a test case where this would matter?

It looks like you've already changed it, but PR #2230 adds a test for this.

brittonsmith · 2019-04-03T18:54:12Z

yt/frontends/ytdata/data_structures.py

@@ -86,7 +86,7 @@ def _parse_parameter_file(self):
            for key in f.attrs.keys():
                v = parse_h5_attr(f, key)
                if key == "con_args":
-                    v = v.astype("str")
+                    v = eval(v)


This is a backward incompatible change. Old datasets won't be loadable. Is there a reason for this?

I did this to fix a test. The ytdata frontend is using the json representation of the unit registry, which I had to change in unyt so I don't think there's a clear way to load old registries written out by yt.units and load them with unyt again.

Are we really supporting loading ytdata datasets accross yt versions? There's no tests for that, for example...

…tic_output.py

matthewturk · 2019-05-08T15:39:53Z

@ngoldbaum -- looks like the only remaining conflict is in appveyor.yml, and all existing tests pass. Does the current state work with older YTData outputs? If so, what else remains before we can merge this in?

ngoldbaum · 2019-05-08T15:43:10Z

There would need to either be support in yt or unyt to load old unit registries. I was hoping to finish that off before I left NCSA but did not get it done unfortunately.

matthewturk · 2019-05-08T15:56:13Z

@ngoldbaum Do you have a suggestion for how we can move forward? Is this something that could reasonably be implemented by someone else?

ngoldbaum · 2019-05-08T15:56:53Z

I think so, yes. Help would be greatly appreciated. Support could either live in yt or unyt, whichever is easier.

brittonsmith · 2019-05-08T16:17:26Z

We now have tests in master for loading old ytdata datasets, so we either have a block here or in merging yt-4.0 into master. I'd be happy to look into this, but I don't really know where to look. @ngoldbaum, would it be possible to lay out what needs to be done to make sure old ytdata is still loadable?

ngoldbaum · 2019-05-08T16:20:35Z

The json unit registry loader in unyt would either need to be patched to be able to load old unit registries or there would need to be a shim in yt that can parse the old json unit registries and emit a new unyt UnitRegistry object.

brittonsmith · 2019-05-08T16:30:31Z

Can you say a bit about how the registries are different?

ngoldbaum · 2019-05-08T16:42:36Z

The relevant change is this one: yt-project/unyt@6283d20. Also take a look at the explanation in the pull request: yt-project/unyt#81

The change in the json output format is because we no longer use srepr to represent the symbol objects and instead just save the name of the symbol object. That means we go from saving e.g. Symbol('mass', positive=True) in the output to saving just mass.

With the old way of loading unit registries, sympy creates a distinct symbol object that satisfies == equality but not is equality. This is problematic because we use is equality a bunch of places inside unyt for speed. My fix was to make the dimensions in unyt.dimensions be singletons and use them as the namespace for restoring the unit registry json.

I think you'd need to do a similar trick with the old json objects, parsing e.g. Symbol('energy', positive=True) to just 'energy', and then either creating the unit registry manually or using the unyt loader.

I'm very sorry that I didn't do this myself and finish this before leaving. I'm pretty much completely burnt out on yt development and every time I tried to start on this I'd distract myself and feel guilty about it. I should have asked for help earlier and I'm sorry that I didn't.

ngoldbaum · 2019-06-25T16:14:49Z

yt-project/unyt#93 should hopefully unblock this

matthewturk · 2019-07-02T18:48:31Z

Looks like appveyor dies with:

======================================================================
FAIL: yt.frontends.stream.tests.test_stream_particles.test_load_particles_with_data_source
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Miniconda36-x64\envs\test\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "C:\projects\yt\yt\frontends\stream\tests\test_stream_particles.py", line 338, in test_load_particles_with_data_source
    assert in_cgs(get_cu(ds1, dim)) == in_cgs(get_cu(ds2, dim))
AssertionError

I'm not entirely sure but maybe this is just a test that doesn't need to be run anymore?

ngoldbaum · 2019-07-02T19:57:19Z

No the test probably needs to be fixed, it’s been a while since this PR was updated. I didn’t actually try running the test locally yet.

ngoldbaum · 2019-07-02T20:59:31Z

I've opened yt-project/unyt#96 to fix the failing test. Unfortunately we're going to need another unyt version bump to get the fix here in yt.

matthewturk · 2019-07-03T18:31:45Z

AWESOME!

brittonsmith

Let's do this.

Nathan Goldbaum added 30 commits June 22, 2018 13:02

it imports!

f91fe57

delete exceptions that have been moved to unyt

33b358f

Handle the fact that to_equivelent isn't needed to MKS->CGS conversio…

f225f01

…ns anymore

use ampere as the base MKS current unit for the code unit system

5ecd0ba

fix more tests

4bd5246

more test fixes

5812ccc

Merge branch 'yt-4.0' into unyt

fc6cb28

fix flake8 errors

01098f8

remove unused import

dfda3f6

ensure unyt is installed on CI

f658fac

Merge branch 'yt-4.0' into unyt

0851bcb

update for changes in unyt

db4cab6

use the new add_constants and add_symbols functions in unyt

cb475cb

add yt.units.unit_symbols

affd2c5

create the dataset's unit registry with a unit system

fd30b95

fix initializing cosmology simulations

e55ba64

add ds.units to resolve issues with fields not using the correct unit…

6143a62

… registry

apply units via multiplication to fix issue in deposit fields

d058a73

fix issue with cosmology object initialization

797cfab

fix magnetic field units test

8fe11b9

fix issues loading a FIRE cosmology dataset

0a2c005

fix issue with fields from the plugins file tests contaminating other…

aa8320a

… tests

remove h5py string saving hack

8108b17

ensure region left and right edges are float64

c04d9e3

fix arbitrary grid data object tests

f36bee8

add explanatory comment about default unit choices

fda7286

make sure mesh_id field isn't detected for particle datasets

87fe717

improve formatting

191056c

fix test setup for ray creation test

e21a255

only do particle drived quantities for datasets with nbody particles

ae1ee63

matthewturk reviewed Apr 3, 2019

View reviewed changes

brittonsmith reviewed Apr 3, 2019

View reviewed changes

Nathan Goldbaum added 4 commits April 4, 2019 14:19

update yt units docs

cf9c889

don't associate the cosmology object with a unit registry

886ea6c

try to get units before physical constants in the UnitContainer

6d4c78c

move UnitContainer, SymbolContainer, and ConstantContainer out of sta…

22e96be

…tic_output.py

ngoldbaum mentioned this pull request Jun 25, 2019

Support loading json unit registries produced by yt.units yt-project/unyt#93

Merged

ngoldbaum added 3 commits July 2, 2019 13:16

fix loading old ytdata outputs

44605ed

Merge branch 'yt-4.0' into unyt

820da2e

update minimum required unyt version

9a63245

ngoldbaum changed the title ~~[WIP] Unyt~~ Unyt Jul 2, 2019

ngoldbaum added 2 commits July 3, 2019 10:13

bump minimum required unyt version

459d419

fix failing test

6613bc6

matthewturk approved these changes Jul 3, 2019

View reviewed changes

jzuhone approved these changes Jul 3, 2019

View reviewed changes

brittonsmith approved these changes Jul 4, 2019

View reviewed changes

matthewturk merged commit e8f5030 into yt-project:yt-4.0 Jul 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unyt #2219

Unyt #2219

ngoldbaum commented Mar 26, 2019

matthewturk left a comment

matthewturk Apr 3, 2019

matthewturk Apr 3, 2019

ngoldbaum Apr 3, 2019

matthewturk Apr 3, 2019

ngoldbaum Apr 3, 2019

matthewturk Apr 3, 2019

matthewturk Apr 3, 2019

matthewturk Apr 3, 2019

matthewturk Apr 3, 2019

ngoldbaum Apr 3, 2019

brittonsmith Apr 3, 2019

ngoldbaum Apr 3, 2019

brittonsmith Apr 5, 2019

brittonsmith Apr 3, 2019

ngoldbaum Apr 3, 2019

matthewturk commented May 8, 2019

ngoldbaum commented May 8, 2019

matthewturk commented May 8, 2019

ngoldbaum commented May 8, 2019

brittonsmith commented May 8, 2019

ngoldbaum commented May 8, 2019

brittonsmith commented May 8, 2019

ngoldbaum commented May 8, 2019

ngoldbaum commented Jun 25, 2019

matthewturk commented Jul 2, 2019

ngoldbaum commented Jul 2, 2019

ngoldbaum commented Jul 2, 2019

matthewturk commented Jul 3, 2019

brittonsmith left a comment

Unyt #2219

Unyt #2219

Conversation

ngoldbaum commented Mar 26, 2019

matthewturk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewturk commented May 8, 2019

ngoldbaum commented May 8, 2019

matthewturk commented May 8, 2019

ngoldbaum commented May 8, 2019

brittonsmith commented May 8, 2019

ngoldbaum commented May 8, 2019

brittonsmith commented May 8, 2019

ngoldbaum commented May 8, 2019

ngoldbaum commented Jun 25, 2019

matthewturk commented Jul 2, 2019

ngoldbaum commented Jul 2, 2019

ngoldbaum commented Jul 2, 2019

matthewturk commented Jul 3, 2019

brittonsmith left a comment

Choose a reason for hiding this comment