Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use astropy units #29

Open
wtbarnes opened this issue Jul 20, 2021 · 2 comments
Open

Use astropy units #29

wtbarnes opened this issue Jul 20, 2021 · 2 comments

Comments

@wtbarnes
Copy link

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
Copy link
Author

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

@Goobley
Copy link
Owner

Goobley commented Jul 21, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants