-
Notifications
You must be signed in to change notification settings - Fork 55
feat: add adaptiveScientific mode
#268
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 7648875.
| // If we have some fractional digits *and* an exponent, we need to | ||
| // adjust the whole part to include the fractional part. | ||
| // 1.23e4 -> 123e2 | ||
| if (exp !== 0 && fractionalPart) { |
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 code seems wrong -- why would we ever want this?
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.
To avoid having both a fractional part and an exponent. As the comment says, it's to have "123e2" instead of "1.23e4".
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.
Put differently: how can I avoid / opt-out of this behavior?
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.
I always either want the decimal version of a number (when numbers are within the avoidExponents range), or normalized scientific notation.
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.
ah.... OK, so you want a scientific notation with an avoidExponentsRange, a non-normalized scientific notation.
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.
value | desired | auto | scientific |
-------- | --------------- | ------------------- | ------------------ |
1 / 7000000 | 0.000000142857... | 0.000000142857... | 1.42857... * 10^-7 |
1 / 70000000 | 1.42857... * 10^-8 | 142857 ... * 10^-23 | 1.42857... * 10^-8 |
- Within the
avoidExponentsRange, we want the "auto" behavior - Outside of the
avoidExponentsRange, we want the "scientific" behavior
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.
I'm happy to put up a PR implementing an option so that we can have this behavior, I'm just not sure what the API design should be.
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.
"adaptiveScientific": Uses fixed decimal notation within a specified exponent range and strict scientific notation otherwise
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, I fixed up this PR to document and add such an option.
|
The first test is incorrect: 1 / 7000000 is |
|
@arnog Should I file a new issue? I interpreted bug 245 as meaning that the input may be in scientific notation. Also, Google shows that Regardless of #245 , the block of code I delete in this PR seems obviously wrong, unless I am missing something. The tests I add to this PR will both fail on master. |
|
It seems like you are looking for a notation which is neither It would be helpful if you could file a new issue describing the notation you're looking for, either with a definition of this notation explaining the relationship between the mantissa and the exponent. |
adaptiveScientific mode
| } else if (options.notation === 'scientific') { | ||
| result = serializeScientificNotationNumber(num.toExponential(), { | ||
| ...options, | ||
| avoidExponentsInRange: null, // Scientific notation should always use exponents |
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.
I think this resolves #245
This PR adds a new
adaptiveScientificmode which is a combination of theautoandscientificmodes. Disclosure: this PR was written by Claude 4.5 Opus in planning mode, and manually iterated