-
Notifications
You must be signed in to change notification settings - Fork 108
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
Fix label_number_si() to use SI prefixes #235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable overall, but will need to wait for a bigger release since it will need some decent marketing around the change.
It's been a while since I looked at this, so could you please give me a brief summary of the overall goals and what's changed since last time I looked? |
Hi @hadley - sure thing! Overall Goals
I think it'd be good to retain support for the short scale, but there are some additional complexities to consider. First, many countries use the alternative long scale or another system (e.g. Indian numbering system). Because it's particularly common to use the short scale in finance (e.g. $1M), I decided it makes most sense to add this feature to The goal of this PR is twofold:
Changes since last review
|
https://en.wikipedia.org/wiki/Dimensionless_quantity It's entirely reasonable to use SI prefixes to represent dimensionless quantities, such as counts and ratios. |
Thank you very much for this work, David! It'll make |
@sjackman Thanks for your feedback about dimensionless quantities, though my concern was specifically about using SI prefixes with dimensionless quantities. I've never seen this done before and I couldn't find any examples on that Wikipedia page. This might be getting a little pedantic now, but I looked in the official publication of the Bureau International des Poids et Mesures titled "The International System of Units (SI)". In section 3.1, they state:
In the vast majority of cases, people will want to set the Just to reiterate that I don't mind being overruled on this, if people feel strongly about this. I know it must sound like I'm very passionate about SI units right now! 😆 |
I'm glad that you're passionate about SI units! My preference is for Does the official publication make any recommendation as to how to represent dimensionless quantities, other than don't? Which is actually pretty funny. 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, and for the detailed explanation of what's changed. Overall, it looks great 😀 I have a few minor comments below, but can you please also add a NEWS bullet making it clear that this is a breaking change, and briefly justifying why (i.e. it previously didn't actually use SI units)
This wasn't actually doing anything, because user inputs were overwritten.
There are some types of dimensionless quantities, where the recommendation is to use a derived unit (not a base SI unit). For example, a measurement of plane angle would use radians (which expressed in terms of base SI units is meter / meter). In this situation, we can use the SI prefixes (e.g. milliradians or mrad). They don't give explicit recommendation for situations where there is no appropriate unit (e.g. count) but the number is significantly different from 1. I think a reasonable approach would be to use scientific notation. There is already the scales::label_scientific() formatter for this purpose. |
Hi @hadley, I've updated with your feedback. Before updating the NEWS, I was just hoping for your feedback on this point:
The official recommendation from the standards organization is not to use SI prefixes without a unit, so I think 99% of usage will require that this argument is set. My hope is to reduce user error by prompting for this argument. They could still set @sjackman is in favor of retaining the default value of NULL (i.e. no unit). This has the advantage that existing code still produces a plot (though the result is changed). I think we each have moderate strength of feeling for our own preference, but are both willing to accept the alternative option. So I'm really just looking for a decision on how you'd like {scales} to handle this. There's a bit of discussion above, if you'd like to read more context. |
Making it required has the nice property that existing users will get an error, forcing them to look up the docs and discover that its changed. |
Hi @hadley - I think I'm finished with this PR now. I tried to follow the NEWS style guide, but please let me know if I need to update anything. Thanks! |
Thank you, David! ✨ |
Fixes #234. Fixes #301.
TLDR:
label_number_si()
is currently providing short scale abbreviations instead of SI prefixes.Both SI prefixes and long/short scale abbreviations are solutions to the same problem – how to specify large values more concisely. SI prefixes are now favored in most domains except finance. Abbreviations of long/short scales are not standardized (e.g. million could be "M", "m", "mn" in different circles) and depends on locale.
I've done my best to adjust the interface, but would love to hear feedback on how I could improve!
Here's what changed:
label_number_si()
unit
argument was promoted to a required argumentscale
argumentaccuracy
changed toNULL
(i.e. auto precision)sep
argument (wasn't actually doing anything)label_dollar()
rescale_large
argumentrescale_short_scale()
andrescale_long_scale()
built-in values, to use with this argumentlabel_bytes()
scale
argument (would've been passed by...
)