Make Arepo units play nice with unit_base

[jzuhone]

PR Summary

GadgetDataset and its various subclasses allow for a unit_base argument to be passed in to set units if they aren't in the dataset. This was not working for ArepoHDF5Dataset, which usually (but not always) has the units in the file--if they weren't in the file, then whatever was in unit_base was simply being ignored, mainly because the attribute _unit_base was simply being clobbered by ArepoHDF5Dataset and erasing whatever had been inputted by the user.

This fixes it by allowing Arepo to figure out what kind of units it has, save them, and then update the _unit_base dict with its values (if it has any) before letting the superclass GadgetDataset check them.

However, whatever units are in the dataset must take precedence--and we will warn the user about this.

PR Checklist

• New features are documented, with docstrings and narrative docs
• Adds a test for any bugs fixed. Adds tests for new features.

[matthewturk]

I know you expressed frustration in your commit message with how messy this is, but ... do you think there's any way to make it simpler? I am somewhat skeptical. Good work covering all the bases.

[chummels]

Perhaps I'm misunderstanding this. When I run this code locally, it still tells me that the "Arepo header is missing UnitLength_in_cm!" (and other warnings), but then it seems to set them OK from the unit_base that I included in the yt.load() call. Well, it seems to get the distance unit correct, but I think the mass unit is wrong. But that doesn't seem related to what you've changed here, since all of them should work equally well. I'm a bit confused by this behavior.

[chummels]

arepo_unit_base is defined as what is in the header (since it comes from self._get_uvals(). But if the header has nothing defined, then this conditional automatically gets skipped. So it seems weird that there are lines like: "Overwriting X in unit_base with what we found in the dataset." Isn't the unit_base supposed to override whatever is (or isn't) in the dataset? Or maybe I'm misunderstanding.

[jzuhone]

I think that unit_base should define units when there aren't any. units_override works for overriding units--it currently doesn't work for Gadget-based data, but we could make it work in a separate PR. I had avoided doing it before because I wasn't sure how the two things would work with each other, but now I think they could.

[jzuhone]

Now that I think about it, we could probably make this simpler by letting unit_base do overrides. I'll think about it more.

[chummels]

Yeah, I never understood why there was a separate unit_override kwarg from the unit_base kwarg. I would think they both serve the same purpose, which is to set the units for a dataset. I would assume that if a user is specifying a user_base, then the dataset lacks one, or the that user knows better, so it should clobber the existing one? But maybe that's not a good assumption. But that's probably for a different PR.

The only other suggestion I have for this one is that when one currently runs yt to load up a dataset using the unit_base, yt gives warnings saying that "Arepo header is missing UnitLength_in_cm!", even when you're setting them properly with the unit_base. It might be worth either suppressing the "missing" warning or adding an additional yt log statement that yt is using the values prescribed in the unit_base to alleviate any freakouts by the user.

[jzuhone]

I'm just going to remove that log message because it's not strictly necessary.

[jzuhone]

Can you post the code and log messages for this? I'm not sure what you mean when you say the length unit is ok but the mass unit isn't.

[chummels]

Sure, i can post it. The main reason I think the mass is wonky is when I plot a ProjectionPlot of the density for this cosmological field, I correctly get the box size as being 100 Mpc across now, but the density plot is showing like a few thousand grams / cm**2 as the zlim, which is low by a factor of around 1e20. I'll send what I mean, but It'll be later tonight.

[jzuhone]

Just to check--the density units in your plot sound potentially reasonable? I don't know what the context is, but if the gas density is on the order a proton mass (give or take an order of magnitude) per cc, and then in the projection plot the path length is 100 Mpc, it seems like for g/cm**2 that's about the right numerical value.

[jzuhone]

Eg rho*L ~ 1.0e-24 * 100 * 3e24 ~ 300 g/cm**2

[chummels]

So when I reviewed things last night, my brain wasn't on straight. My apologies. You are absolutely correct that the density values are reasonable. I was somehow thinking in density instead of column density. In comparing the projection plots made with this dataset to other non-arepo datasets of a similar nature, everything checks out. Thank you!

[jzuhone]

And I've reverted back to my original thought (based on looking at the code more) that without substantial changes to unit_base we can't make it do an override (it's just not in the right order).

[chummels]

And I've reverted back to my original thought (based on looking at the code more) that without substantial changes to unit_base we can't make it do an override (it's just not in the right order).

OK, no need to worry about it for now. That can be for a future refactor. Do you think it is possible to either suppress the warning that the params are missing, or add another message saying that yt is using the new values from unit_base? Or perhaps that's too much chatter in the logger? Other than that, this PR looks golden! Thanks so much for writing this up, @jzuhone !