merging yt-4 into master

[matthewturk]

This is a reopening of #2172 on my repository. I'm removing WIP because, although it's still a work in progress, it's getting close enough that I want others to check in with it once in a while.

[matthewturk]

What's currently killing me is that my conda environment doesn't have dbm.gnu and that's what the shelve'd answer results are in. Sigh.

[matthewturk]

I verified the images are all correct and have committed the correct versions.

We probably need to get the image answer tests somewhere other than the main repo at some point, but I do like the new system for inspecting and verifying them.

[Xarthisius]

@yt-fido test this please

[matthewturk]

@ngoldbaum so today, @Xarthisius, @munkm and @samwalkow and I were looking at the test results and directly comparing them, and we got some strange results. I genuinely don't know how to interpret these.

Is it possible that this is simply because the cm in yt 3 and the cm in yt 4 don't resolve to being the same object? Or something along the lines of a fundamental unyt / yt3-units incompatibility?

https://gist.github.com/matthewturk/38d36d8cba8127d0faf4916950648f1c

Thanks for any ideas.

[matthewturk]

Ah, we dug into it a bit more -- and, it looks like both resolve the unit registry as being from unyt when both are imported. And, the registry has different values for cm:

>>> r_new['cm']
(0.01, (length), 0.0, '\\rm{cm}', False)
>>> r_old['cm']
(1.0, (length), 0.0, '\\rm{cm}')

[ngoldbaum]

I’m not sure. Maybe? Can you make a self-contained example where you save data with yt.units and load it again later with unyt that hits this problem? There’s a code path in unyt that deals with data saved by yt.units, maybe we need to do some additional massaging of the data there to get things loaded correctly?

Please feel free to open an issue in unyt about this with details and optimally steps
I can use to reproduce. I’m in Austin on a work trip this week but hopefully I’ll be able to get to it soon if you can’t come up with a fix.

[ngoldbaum]

Yes, unyt used an MKS unit system internally, yt.units uses CGS.

Hmm I wonder if the problem is that the unpuckling code in unyt isn’t applying the same transformations that the json loading code does for data saved by yt.units:

https://github.com/yt-project/unyt/blob/master/unyt/unit_registry.py#L265

[matthewturk]

@ngoldbaum We believe that it's because of

yt-project/unyt#2

where the default unit system was changed. If the unit definitions internal to a unit registry are stored with respect to the "default" and the "default" is then changed, this will change the results.

There's an elaborate analogy one could make about changing parallel universes between one where the golden meter stick is used and the golden centimeter stick is used but not knowing they use different golden sticks, but it's doesn't make for very exciting prose.

[ngoldbaum]

Yeah I think there needs to be some logic here that detects when we’re loading data saved by yt.units:

https://github.com/yt-project/unyt/blob/master/unyt/array.py#L1928

You should be able to detect it in the same way as the json loading code I linked to earlier. Ping me on slack or here if you have any questions :)

[ngoldbaum]

yt-project/unyt#93 is also probably relevant

[chummels]

@cphyc OK, awesome! Thanks for looking at that. I guess the dataset is just a bit wonky. So, it seems like yt4 is doing the right thing here. In my mind, that means all the frontend tests are correct, and the only one that needs to be checked is the snipshot test after regenerating it by incorporating the add_sph_fields() functionality.

[matthewturk]

The before and after plots with the different mu unicode symbol are here:

As you can see, the change in the unicode number has caused the mu to be italicized, which is likely because now it can be, or something, I am not sure. Whatever. I am not going to dip back into that well just yet, and instead, going to push forward with the answer verification.

[matthewturk]

Wait, forget that last comment, I read the namesb ackwards. Now it's not italicized. So that's good.

[matthewturk]

This new degF thing is unfamiliar to me, but might be a result of running off unyt master. Maybe?

[Xarthisius]

@yt-fido test this please (regenerating xray fields tests, due to missing emissivity file)

[Xarthisius]

@yt-fido test this please (regenerating ytdata tests since now they actually work, at least half of them...)

[matthewturk]

I've pushed a handful of changes that should both fix a couple tests and show a couple more tests that don't work.

owls_subfind has not been updated to use the new start/stop machinery; because it's already a complicated beast with internal offset calculations, I've opted to fix this by allowing infinite chunk sizes in #2658.

The two tipsy tests test_pkdgrav and test_gasoline_dmonly that were failing should be partially passing now; the particle_types_raw attribute should have been used, as it reimplements a few things that are in sph_answers and nbody_answers and were not updated. Note that it should now fail with fields-not-found errors, or something, and we should change to use the new machinery.

One issue I am having with pkdgrav (specifically) right now is that I am unable to load it when I use the arguments given in the testing file. It needs a value for h in the default unit registry, but one does not exist, and so the bootstrapping logic for setting up cosmology and units is out of sorts. I am working on addressing that in a different pull request.

I filed a handful of issues today while working on this: #2659, #2657, and #2656.

[Xarthisius]

@yt-fido test this please

[matthewturk]

Thanks to @Xarthisius for all the work he put in on this. Hopefully this most recent push will reduce the number of failing tests to a single one, which is the ParticleSelectionComparison for pkdgrav. I'm not yet sure why, but it's returning empty arrays in some cases. Working on that...

[munkm]

🎉 🥇 🎈 🎊 🎈 🥇 🎉