Change the default unit system to MKS #2
Conversation
There was a problem hiding this comment.
Left some thoughts inline.
For the docs, I made some tables describing the unit system:
http://unyt.readthedocs.io/en/latest/usage.html#other-unit-systems
I guess once we're done hashing out what we want to make the default set of base units you can update that?
Tests will be great! I think I can make it so the code coverage bot runs against your pull request if you think that would be helpful. You can also generate a local coverage estimate by running tox in the root of the repo and looking in the htmlcov/ folder. For this estimate to be 100% in sync with the bot's estimate you probably need at least both python2.7 and 3.6 installed - there are some code paths that are 2.7-specific.
Also thanks so much for working on this! If at any point you get tired of messing with this let me know and I'll just merge it and we can iterate from there. I'm still finishing up the docs so I'm not quite ready to publish the first release on pypi. Would you like this to go in before that?
ping @bwoshea who originally brought this up.
unyt/unit_systems.py
Outdated
| cgs_ampere_unit_system["magnetic_field_cgs"] = "gauss" | ||
| cgs_ampere_unit_system["charge_cgs"] = "esu" | ||
| cgs_ampere_unit_system["current_cgs"] = "statA" | ||
| #: The internal base unit system: CGS with extra SI units |
There was a problem hiding this comment.
Possibly crazy idea: why don't we just use SI?
That'll make integrating with unyt back inside yt slightly harder but that's probably worth the effort to make unyt easier to use for a random non-astronomer user. You'd be able to work with CGS units pretty easily anyway.
Not something that needs to happen in this PR of course.
There was a problem hiding this comment.
To be honest, I think that this is what we should do. I suspect that integrating back into yt will actually not be that hard--it may actually be all internal to unyt. We'd have to change the default base units and the unit_lookup_table, and maybe one or two other things.
unyt/dimensions.py
Outdated
| @@ -27,12 +27,16 @@ | |||
| angle = Symbol("(angle)", positive=True) | |||
| #: current_mks | |||
| current_mks = Symbol("(current_mks)", positive=True) | |||
| #: amount | |||
| amount = Symbol("(amount)", positive=True) | |||
There was a problem hiding this comment.
I'm not sure how I feel about needing to define this. There are certainly situations where it's convenient to think of a mol as just a number. I don't do chemistry calculations often enough to have a strong opinion though.
unyt/unit_symbols.py
Outdated
| #: lumen | ||
| lm = Lumen = lumen = quan(1.0, "lm") | ||
| #: lux | ||
| lx = Lux = lux = quan(1.0, 'lx') |
There was a problem hiding this comment.
Add Lambert here. Maybe some of the other luminance units as well?
unyt/unit_object.py
Outdated
| if self.dimensions in em_conversions: | ||
| units_string = em_conversions[self.dimensions][0] | ||
| else: | ||
| raise UnitsNotReducible(self, unit_system.name) |
There was a problem hiding this comment.
If you add luminance and amount to the other unit systems, you don't need to raise this error, you could just convert the data.
Here's my suggestion, for CGS use Lambert as the default luminance unit. Otherwise use candelas (unless there's an obvious other choice for the other unit systems?). Arguing for this: we also use "radian" as the default angle unit for all the other unit systems. As far as I can tell the units for luminance are weird.
Is it really helpful to raise this error? If we make this one concession to keeping things more SI-like by default I think it will be easier to use the library.
|
Also sorry it looks like the tests are broken. That might be my fault. I pushed a bunch of changes to the equivalency system last night that I guess you needed to rebase on? On the plus side there are in-place equivalencies now :) And lots more tests. There are a couple bugfixes that I want to backport for yt.units that the new tests caught. |
jzuhone
force-pushed
the
si_units
branch
5 times, most recently
from
a66ccb6 to
9e61d1f
Compare
…-ampere' base unit system (it's internal and is not supposed to be used by users).
unyt/equivalencies.py
Outdated
| base_unit = unit_str | ||
| return base_unit | ||
|
|
||
| def check_em_conversion(units): |
There was a problem hiding this comment.
this should be prefixed with an underscore so it doesn't end up in the public API
There was a problem hiding this comment.
Does this function have much overhead? It looks like it gets called during all of the unit conversion functions so optimally it would be very cheap and not add much overhead to those functions.
unyt/array.py
Outdated
| em_units, em_us = check_em_conversion(self.units) | ||
| if em_units is not None and em_us == unit_system: | ||
| to_units = em_units | ||
| equivalence = em_us |
|
|
||
| ret = a/b | ||
| assert ret == 0.5 | ||
| assert ret == 50000. |
There was a problem hiding this comment.
I think I'm being dumb, but is this correct?
There was a problem hiding this comment.
Ah, it's because now the base_value of gram is .001, as it should be. Apologies for the dumbness.
I think this is ready to merge? Any objections?
There was a problem hiding this comment.
I don't have any, but I wrote the PR, so...
We should maybe ping @matthewturk @chummels @brittonsmith @Xarthisius
There was a problem hiding this comment.
@matthewturk @chummels @brittonsmith @Xarthisius sorry to ping you again, but I just updated the title and description of the PR, so if you started looking it in the past few minutes, you should read the title and description again--the changes are pretty important.
There was a problem hiding this comment.
I have not reviewed the changes in detail, but I am fine with making this change.
There was a problem hiding this comment.
When we decide to make yt depend on unyt in yt 4.0, we should be able to maintain cgs as the default in yt with only a little bit of trouble.
There was a problem hiding this comment.
Yup, another thing we might eventually do is make the default unit system used by e.g. in_base and friends is configurable.
There was a problem hiding this comment.
OK, so just to clarify:
unyt is an independent library doing all of the unit-based things that yt has done in the past. This PR makes unyt default to MKS instead of cgs as it has been previously (and is in yt). This PR will change unyt, but not the behavior in yt. But down the line, (i.e., yt 4.0), yt will potentially use unyt as a dependency and it will effectively make yt's units to default to MKS). Is this right?
If my assessment above is correct, I'm fine with these changes. When it comes time to release yt 4.0, I think we should make a clear and easy path for all of the users of yt to use cgs, since most of the user-base are astronomers who will want to continue having cgs as their default unit system.
There was a problem hiding this comment.
yt will potentially use unyt as a dependency and it will effectively make yt's units to default to MKS). Is this right?
I'd replace potentially with almost definitely :)
since most of the user-base are astronomers who will want to continue having cgs as their default unit system.
Yup. I'm certain the astrophysics simulation domain context will default to CGS units.
There was a problem hiding this comment.
@chummels my hope is that we can make it so that for the astrophysics frontends the users will see no change without having to do anything.
jzuhone
changed the title
jzuhone
changed the title
|
OK, I'm merging this. @jzuhone thank you so much for your effort on this, it will make |
This PR changes the default unit system of
unytto SI/MKS. This requires touching nearly all the physical ratios, most of the physical constants, and the unit lookup table, as well as a bunch of other code.It also:
I think it's probably appropriate to also add to the docs a bit of a discussion about the SI system, haven't done that yet. Should probably also add some more testing coverage which uses these new units and dimensions.