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

Proposal: new exponentformat which uses "milli" prefix #5111

Closed
ignamv opened this issue Aug 31, 2020 · 9 comments · Fixed by #5121
Closed

Proposal: new exponentformat which uses "milli" prefix #5111

ignamv opened this issue Aug 31, 2020 · 9 comments · Fixed by #5121

Comments

@ignamv
Copy link
Contributor

ignamv commented Aug 31, 2020

Can someone please look over my pull request to see if it's suitable for the main repo?

ignamv#1

@archmoj
Copy link
Contributor

archmoj commented Aug 31, 2020

Looking good to me.

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Aug 31, 2020

I see in the constant array SIPREFIXES that k and m are there already... it just seems like a bug to me that exponentformat="SI" doesn't use these.

@nicolaskruchten
Copy link
Contributor

PS @ignamv thanks for the PR! I'm just wondering if we can't just fix this in SI instead of introducing a new mode :)

@alexcjohnson
Copy link
Collaborator

The existing SI mode is working as intended. Not using the m prefix - but using all the others - is pretty typical on log axes because the m is too often confusing (vs M) and doesn't shorten the labels much (100m is longer than 0.1). For k it's a little more subtle, we don't use it for 1k but we do for 10k. This I don't feel that strongly about, we could consider it a bug, but I do think there's a logic to the current behavior: the longest pure-power labels you have otherwise are 100k and 100M etc, which are exactly as many characters as 1000, so we defer to readability and use unmodified numbers for just one more power of 10.

All that is to say I think the current behavior makes sense as a default for SI. But I can see wanting to change that and always use a prefix - or, for that matter, keeping the numbers unmodified longer (or using prefixes more extreme than f and T, beyond which we currently switch to power notation - because I didn't deem those prefixes sufficiently well known when this feature was originally written - but that probably would need to be a totally separate attribute).

So what if we make a new attribute to substitute in for the 3 here:

if(Math.abs(rangeexp) > 3) {

something like ax.minexponent? For your use case @ignamv you'd set it to 0, but someone who doesn't even want to use k or µ might set it to 6, or something between even.

@ignamv
Copy link
Contributor Author

ignamv commented Aug 31, 2020

So what if we make a new attribute to substitute in for the 3 here

Sounds cleaner! I'll give it a go.

@ignamv
Copy link
Contributor Author

ignamv commented Sep 1, 2020

I've added the new attribute and removed the eng exponentformat. I'm not familiar with this codebase so I based the new code on what I found for the exponentformat attribute.

ignamv#1 (comment)

@alexcjohnson
Copy link
Collaborator

@ignamv looks good! Feel free to make a PR to the main repo. It'll need a test or two before it's ready to merge, probably one setting minexponent to zero (the case you care most about) and another setting it large like in ignamv#1 (comment)

This is a good model to start with:

var textOut = mockCalc({
type: 'log',
tickmode: 'linear',
exponentformat: 'B',
showexponent: 'all',
tick0: 0,
dtick: 1,
range: [-18.5, 18.5]
});
expect(textOut).toEqual([
'10<sup>\u221218</sup>',
'10<sup>\u221217</sup>',
'10<sup>\u221216</sup>',
'1f', '10f', '100f', '1p', '10p', '100p', '1n', '10n', '100n',
'1μ', '10μ', '100μ', '0.001', '0.01', '0.1', '1', '10', '100',
'1000', '10k', '100k', '1M', '10M', '100M', '1B', '10B', '100B',
'1T', '10T', '100T',
'10<sup>15</sup>',
'10<sup>16</sup>',
'10<sup>17</sup>',
'10<sup>18</sup>'
]);

@ignamv
Copy link
Contributor Author

ignamv commented Sep 3, 2020

Thanks! I've added both tests and they seem to work.

@archmoj
Copy link
Contributor

archmoj commented Sep 3, 2020

@ignamv Great news! Please move forward and submit a PR.

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 a pull request may close this issue.

4 participants