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

add 'as' as arcsecond for alias with prefixes enabled #28

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

mileslucas
Copy link
Contributor

It is extremely common in astronomy to use arcseconds. Very commonly, we work with quantities like "milliarcseconds" and "microarcseconds" for the super small angles on the night sky. We've run into this issue in (JuliaAstro/UnitfulAstro.jl#7). This PR adds a new unit "as" (ArcsecondShort struct name) which has prefixes enabled. The following conversions are now trivial-

julia> using Unitful, UnitfulAngles

julia> plate_scale = 0.91u"mas/mm"
0.91 mas mm⁻¹

julia> plate_scale |> u"rad/m"
4.411804498096778e-6 rad m⁻¹

julia> ustrip(ans)  deg2rad(ustrip(plate_scale) * 1e3 / 3.6e6)
true

I've also added as to the testing units, and have added extra tests to ensure the prefixes are accurate alongside what Unitful provides.

Current Problems

There are two problems to address that I would like feedback on, if possible

  • u"as" is technically valid as "attoseconds". I don't know a good workaround for this- if there was a way to disable u"as" while still allowing, e.g., u"mas" that would be perfect, since it's easy enough to write out u"arcsecond" if the prefix isn't needed.
  • I can't find any clear examples of essentially aliasing a unit in Unitful.jl, although we are aliasing Arcsecond with this new ArcsecondShort unit. I don't know if this is the appropriate way and I don't love it, since it seems very easy to get mixed up.

One of the alternatives would be adding two bespoke units for millarcsecond and microarcsecond, but that feels very ad-hoc and adds more potential for bugs between the three Unitful libraries I've mentioned

@yakir12
Copy link
Owner

yakir12 commented Aug 26, 2021

Thanks a lot for this. I don't have a lot of time to invest in this package (also see this comment), but I trust your judgement, so I'll merge this u"as" is (heh).

@yakir12 yakir12 merged commit 5d901bf into yakir12:master Aug 26, 2021
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

Successfully merging this pull request may close these issues.

2 participants