Lightweaver icon indicating copy to clipboard operation
Lightweaver copied to clipboard

Use astropy units

Open wtbarnes opened this issue 2 years ago • 2 comments

I know that this has come up in conversation at some point, but I think it would be really useful to use astropy units throughout this package, particularly at the user interface layer.

I fully realize that introduction of units is not a trivial addition and can also involve some performance penalties as well. However, the units on some inputs/outputs are sufficiently ambiguous to warrant this. For example, I was modifying the z component of the velocity on the FAL atmosphere and I was not sure what the components on the velocity were (I believe they are SI, m/s).

wtbarnes avatar Jul 20 '21 19:07 wtbarnes

Ah I've just found unit_view on Atmosphere. That is very useful!

wtbarnes avatar Jul 20 '21 20:07 wtbarnes

Indeed, the Atmosphere constructor can also take units too through the .make_*d statuc methods. Internally, everything is converted to SI, with the exception of wavelength in nm. This is discussed a little in the associated paper, which is supplementary to the docs (and probably the most code in an accepted ApJ article :D). This and the .unit_view were a compromise due to performance and a few issues I had with units behaving a little unexpectedly.

Something like .unit_view should probably be added to the spectral output too, and I'll leave this open to address this at some point, as well as improve the documentation.

Goobley avatar Jul 21 '21 10:07 Goobley