Skip to content

Instantly share code, notes, and snippets.

@astrofrog
Last active February 9, 2017 19:30
Show Gist options
  • Save astrofrog/5d4760f3702ebd324855f85f43207bc0 to your computer and use it in GitHub Desktop.
Save astrofrog/5d4760f3702ebd324855f85f43207bc0 to your computer and use it in GitHub Desktop.
Display the source blob
Display the rendered blob
Raw
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
@astrofrog
Copy link
Author

@pllim:

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...

Both good points, I think this needs more discussion before making a decision and will mention these open questions in the PR.

Could you provide an example for "n_models > 1"? If not, I am going to test for that anyway when you are done with coding. But we can also hash it out here if you want.

I will try and add an example here, but it'd be great if you could try that independently.

Have you considered the complexity of compound models with units and the fitting that goes along with those?

No, but if you want to try this out, that would be great. I suspect there is still work to be done there!

Re: "and we carry out the fit as we would without any units" -- I don't understand this statement, as your "x" and "y" both have units...

I meant that the user doesn't have to do anything different from usual for it to work

@astrofrog
Copy link
Author

@nden:

g2 = Gaussian1D(stddev=1*u.cm)

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.

models.input_units_allow_dimensionless=True

I'll look into why this doesn't work with model sets, will investigate!

The behavior with polynomials is a bug, fix forthcoming.

@astrofrog
Copy link
Author

@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.

@pllim
Copy link

pllim commented Feb 9, 2017

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 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment