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 new prefixes #691

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add new prefixes #691

wants to merge 1 commit into from

Conversation

Eben60
Copy link
Contributor

@Eben60 Eben60 commented Nov 4, 2023

2022 four prefixes were added. Two for forming multiples: ronna and quetta ("R", "Q") Two forming submultiples: ronto and quecto ("r", "q").

https://www.nist.gov/pml/owm/metric-si-prefixes

2022 four prefixes were added.  Two for forming multiples:  ronna and quetta.  Two forming submultiples:  ronto and quecto.
https://www.nist.gov/pml/owm/metric-si-prefixes
@sostock
Copy link
Collaborator

sostock commented Nov 6, 2023

Thanks for your contribution!

I think it would be good to add the new prefixes, but there is one aspect to consider: Unitful.rm (rontometre) will collide with Base.rm (function for deleting a file). In principle, this is not a problem (most people will probably use u"rm" instead of importing Unitful.rm), but there is at least a precedent for avoiding these kind of name collisions: the name Unitful.minute was chosen instead of Unitful.min to avoid confusion with Base.min.

Nevertheless, I think we should add these prefixes (they are part of SI after all).

We could consider not exporting the new units from Unitful.DefaultSymbols (this is actually what would happen if we merge this PR as is1), so that users of Unitful.DefaultSymbols don’t run into this collision 2. On the other hand, exporting only some prefixed units and not the others seem inconsistent (and if you just import every symbol from a package you have to expect collisions if that package adds new symbols). Also, Base.rm is probably not as widely used as Base.min.

@giordano What do you think about this?

Footnotes

  1. To export the new units from Unitful.DefaultSymbols, the new prefixes would have to be added here as well: https://github.com/PainterQubits/Unitful.jl/blob/bc5dfb2e87b176274c29c4caec1fd79006c16e17/src/pkgdefaults.jl#L698-L699

  2. There are several packages that use Unitful.DefaultSymbols.

@sostock
Copy link
Collaborator

sostock commented Nov 6, 2023

The test failures on master are unrelated (they are fixed by #689, which was merged after this PR was posted).

@Eben60
Copy link
Contributor Author

Eben60 commented Nov 6, 2023

rm is a commonly used function, and as for rontometer - I'm not sure if any distance having physical meaning can be measured in rontometers.

So personally I'd suggest not exporting the new prefixes from Unitful.DefaultSymbols (any of them - for consistency) and mention that in the documentation.

@sostock sostock added the new units adding new units/dimensions/constants to this package label Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new units adding new units/dimensions/constants to this package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants