Unit compose and equivalencies - #2559

I think this is because Quantity uses these mechanisms to make the equivalencies happen? But I could be wrong... perhaps @mdboom knows?

Yes -- equivalencies in compose are broken. I spent some time looking at this last week, and I think it does make sense to fix this independently of #2544, which doesn't actually introduce any regressions in this regard.

It would seem there is a general problem for equivalencies as applied to units, in that it is not possible to give just a decomposition for equivalencies that involve a scaling (like doppler_*); see also #2496. In that sense, we may have to make a difference between finding units a quantity can in principle be composed into, and an actual composition in those units.

I've attached a fix that works for the examples provided.

@mhvk: I'm not sure I understand your comment that this doesn't work with parameterized equivalencies. The units returned by compose always contain a scale, and will have an appropriate scale for the given equivalency. The find_equivalent_units method (which calls compose under the hood) will already "strip" these scales and provide a list of units the unit can in principle be composed to, where as "raw" compose will include a specific scale for each.

@mdboom - thinking again, my comment may indeed be incorrect. But I'm not sure and perhaps this is easiest addressed by adding a test of what I would, perhaps naively, believe the answer should be:

    x = (u.nm).compose(units=(u.m, u.s), equivalencies=u.doppler_optical(0.55*u.micron))
    assert x[0].bases == [u.m, u.s]
    assert x[0].powers == [1, -1]
    assert_allclose(x[0].scale, (1.*u.nm/(0.55*u.micron)*constants.c).to(u.m/u.s).value)

A propos tests, I also think it would be good to test explicitly that dimensionless_angles() works for arbitrary powers of radian. So, how about also adding the following:

    x = (u.degree**3).compose(units=(u.dimensionless_unscaled,), equivalencies=u.dimensionless_angles())
    assert x[0] is u.Unit(u.degree.to(u.radian)**3)

(maybe this needs an assert_allclose on the scale instead)

For the first example, that makes sense, and the result matches

>>> u.nm.to(u.m / u.s, equivalencies=u.doppler_optical(0.55 * u.micron))
-299247380.8036363

not

>>> (1.*u.nm/(0.55*u.micron)*constants.c).to(u.m/u.s).value
545077.1963636364

which is self-consistent. If that's not correct, than the equivalency is different that what you were expecting even in the to case.

As for the second, compose has never done the sort of partial factoring of powers of the whole unit that that would require. It could, I suppose, but it was a semi-deliberate choice not to, as the search space is already big enough... Plus, there's an interesting ambiguity when doing that, by the equivalency, since rad3 is equivalent to rad2, rad**1 and "dimensionless". I suppose that's technically correct, while surprising.

I think the above points to my problem with doppler_* - here, the unit nm is treated as if it were a wavelength of 1 nm, i.e., as if it were a quantity. But it is not: for units, I would expect results to scale, i.e.,

with u.set_enabled_equivalencies(u.doppler_optical(0.55*u.micron):
    assert u.nm.to(u.m/u.s) == 10. * u.Angstrom.to(u.m/u.s)

But the way the equivalency is written, I see that this is essentially impossible.

Note that this is similar to the temperature units, where there is a well-defined scaling between Fahrenheit, Celcius, and Kelvin, which is all one may care about for the units, but also an offset, which is important for actual quantities.

In any case, I'm not sure whether we can handle this here. In practice, I think it is far more important to address #2496, which is much more likely to cause hard-to-debug problems.

p.s. On dimensionless_angles(), indeed logically .compose would lead to an infinite set of possible alternatives. Obviously, that wouldn't do. Both examples really make me wonder whether we should not remove the option to get new units using equivalencies; maybe those should be used in quantities only...

I'd still like to see this in -- I agree with @mhvk it's not perfect, but I think it's an improvement over what we have. Perhaps a little too large/risky for a bugfix release, so we milestone this for 1.0 instead of 0.4.x?

Up to you.