Repository navigation
Add "compose" method to units. - #579
Conversation
|
False positive on the Travis build failure. |
|
This looks great! I need to look over it in detail, but at first glance, this looks like a very useful addition. |
|
Fantastic--I'm tempted to say we should try to get this into 0.2, though it would be good to get more commentary on it first. |
|
Small bug: I don't think it should be EDIT: I think I understand - |
|
@mdboom - I haven't looked at the code in detail, but is there a reason this doesn't work? |
|
decompose always decomposes to base units, i.e. it only splits units into parts, it can't recompose them back into units (as you're expecting to do with Newton -- not a base unit -- here), so I think what you want to use is compose. Unfortunately, the current version of compose always aims to make a simpler unit expression (where simple is defined as minimizing The first thing compose does is to try to factor I'll experiment with removing the simplification restriction and see if this can be made to work. |
|
Try the latest commit here. And let me know if there's anything else worth doing that this can't do -- this feedback is really helpful. |
|
@astrofrog : Have you had a chance to experiment with this latest version? I think this is a pretty significant upgrade to the unit capabilities -- we should get it right. Also @adrn: Any issues wrt to quantities? |
|
@mdboom - I'll see if I can try this out this week (I'm still out of town, but on the computer from time to time). |
|
@mdboom - I'm playing around with this a bit more, and ran into the following: is this expected? SI works: Also, could you clarify what the difference is between e.g. |
|
Ah, I see. The thing that's throwing it off is the fractional powers. It never gets to anything that looks "simpler" so it sort of gives up. Of course, if it tried To clarify the difference between |
|
Ok -- I have a fix for the fractional powers problem. The nice thing is that |
|
@mdboom - the example I was posting before works nicely now! We may want to think in future about whether we really want to allow any set of units for are a bit silly (and it can be pretty slow). For example, maybe compose should default to a certain basis such as SI or cgs. I noticed one more bug - there appears to be duplicates: Other than that, this looks good to me, so once that's fixed, I think this is fine to merge (since it doesn't change the existing API). |
|
I've attached a commit to deal with the duplicate result problem. I'm not opposed to the default base being SI for compose, but it is a little inconsistent with other things where this default is always "everything" (including user-defined units). I wonder if there's any other ideas... will have to think on that. |
|
@mdboom - I agree, it's something to ponder about, though we don't need to hold up this PR. The current behavior is perfectly fine. |
|
Ok. In my view the only thing remaining for this PR is a quick look-over from @adrn w.r.t. Quantities, since it completely replaces the |
|
@mdboom This looks great! I like the new stuff in |
|
@astrofrog: Rebased. |
|
@mdboom - can we merge this once the tests pass, or was there anything else you wanted to add? |
|
I think this is good to go, if everyone else agrees. |
|
Ok, great! Feel free to merge. |
|
I've already started rebasing the work @astrofrog and I started in #572 off of this. Hopefully it will lead quickly to a solution to that issue. |
|
One problem I'm getting now is this: Should Kelvin be one of the CGS base units? |
|
It looks like we already add |
|
Hmm, would it be overkill to have a test to check that all units can be decomposed into cgs and SI? (how slow would it be?) |
There was a problem hiding this comment.
This looks like a debug statement that was left in?
|
Well there's no reason you can't use K in cgs--the problem is that SI lists it as a base unit and cgs doesn't. You could convert to K to erg by way of Boltzmann's constant and then decompose that to cgs base units, but I don't think anybody would actually want to do that. |
|
I'm fine with adding K as a base unit for CGS. As for checking that all units can be decomposed into CGS and SI, I don't think that's possible. The CGS system has no equivalent of the Ampere in SI, meaning the conversion from Coulomb to Statcoulomb depends on context: We would need to provide equivalencies for this, and the user would need to select which equivalency applied. We could, however, test some subset of units that we know should work. Aside from this, there are also units in our system that FITS defines (such as pixel and voxel) that don't really have SI or CGS equivalents (and are arguably not physical units anyway). |
|
According to Wikipedia anyways mole is actually an SI base unit (which to me seems a bit odd but okay). |
|
@iguananaut - I saw that too, but I think it's nonsense :-) |
|
@mdboom - this seems fine to me! Let's make sure the tests pass on Travis, and if they do, then we can go ahead and merge! |
|
+1 from me |
"decompose" and tries to reduce the number of components in a CompositeUnit. The Quantity.si and Quantity.cgs methods can then much more easily be implemented in terms of this functionality. Also addresses astropy#576 be providing a way for the user to decompose into a particular system. Other changes along the way: - PEP8-ify quantity.py - Privatize the "_simplify" member on units, since it's confusing in relation to "compose" and was never meant to be public anyway.
… complexity of the unit as it works. This allows expressions to me composed into more complex expressions when the set of desired units requires it. (Point raised by @astrofrog in the PR). This is a slightly back-to-the-drawing-board approach for compose.
…that is most useful (simplest results first).
and cgs to include some other tertiary units. Add tests that make sure that all SI units can convert to CGS and vice verse (with the exception of units containing Ampere).
|
I think the mole is an SI unit because chemists want SI units for things like molarity and molality or whatever. So probably not too much of an issue for us |
Add "compose" method to units.
Addresses #576.
Adds "compose" functionality to units -- it's the converse of
"decompose" and tries to reduce the number of components in a
CompositeUnit.
The Quantity.si and Quantity.cgs methods can then much more easily be
implemented in terms of this functionality.
Also addresses #576 be providing a way for the user to decompose into
a particular system.
Other changes along the way:
relation to "compose" and was never meant to be public anyway.
This doesn't have to make it into 0.2, but it doesn't really change the API in any meaningful way (note that none of the tests changed to make this all work). If it doesn't go into 0.2, it might be a good idea to at least privatize "simplify" on 0.2 so that users won't rely on it as a public method.