-
-
Save astrofrog/5d4760f3702ebd324855f85f43207bc0 to your computer and use it in GitHub Desktop.
@pllim - I've now disallowed both of these cases in the PR:
Allowing user to change to any parameter unit without changing value can be potentially confusing. Say, in your Gaussian, if I initialize "mean" to 3 m, and then I set just the unit to cm, naively I might think the mean is now 300 cm. In addition, while allowing user to set parameter unit to whatever desired ensures flexibility, it might not be the right thing to do for a model like blackbody (e.g., it only makes sense for its "temperature" parameter to accept temperature units).
Re: Raising error when setting "mean" as a Quantity to scalar -- An alternate behavior is to assume the original "mean" unit without error (but maybe with UserWarning). That would be more flexible. Although I know I am contradicting myself, because I just argued against flexibility above...
I tried different options but I think that the end of the day that both are potentially confusing.
For the Gaussian1D case, the following definition does not make sense (i.e., setting stddev
to a unit that is incompatible with mean
) but it is allowed, only to cause other exceptions to be raised on evaluation:
>>> g1 = Gaussian1D(amplitude=1*u.Jy, mean=5000*u.AA, stddev=0.1*u.Jy)
>>> g1(5000)
UnitsError: Can only apply 'subtract' function to ...
>>> g1(5000 * u.AA)
TypeError: Can only apply 'exp' function to dimensionless quantities
While I understand that compound model is future work, we also have to be careful such that massive refactoring won't be required in the future in order to make units work with them. In real life, most of us deal with compound models, so if we want to advertise that units are possible in models, we also have to make sure they will indeed be possible for compound models (albeit not implemented in this PR).
Currently:
>>> g1 = Gaussian1D(amplitude=1*u.Jy, mean=5000*u.AA, stddev=10*u.AA)
>>> g2 = Gaussian1D()
>>> gg = g1 + g2
>>> gg(5000)
UnitsError: Can only apply 'subtract' function to ...
>>> gg(5000 * u.AA)
TypeError: Can only apply 'exp' function to dimensionless quantities
or
>>> g1 = Gaussian1D(amplitude=1*u.Jy, mean=5000*u.AA, stddev=10*u.AA)
>>> g2 = Gaussian1D(amplitude=300*u.mJy, mean=30*u.MHz, stddev=10*u.Hz)
>>> gg = g1 + g2
>>> gg(5000 * u.AA, equivalencies={'x': u.spectral(), 'mean':u.spectral(), 'stddev':u.spectral()})
UnitConversionError: Can only apply 'subtract' function to ...
@nden:
This works because of a special rule: 0 doesn't require units to be treated as a quantity. So I think everything is fine here.
I'll look into why this doesn't work with model sets, will investigate!
The behavior with polynomials is a bug, fix forthcoming.