-
Notifications
You must be signed in to change notification settings - Fork 19.8k
fix(axis-log): Enable ticks under 1e-10, fixes #21099 #21107
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
Conversation
Ovilia
left a comment
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.
Please revert changes under dist.
src/scale/Log.ts
Outdated
| function calculatePrecision(value: number): number { | ||
| if (!isFinite(value) || value === 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| const str = value.toString(); | ||
|
|
||
| // Decimals using scientific notation | ||
| if (str.includes('e')) { | ||
| const [coefficient, exponent] = str.split('e'); | ||
| const coefficientDecimals = coefficient.includes('.') | ||
| ? coefficient.split('.')[1].length | ||
| : 0; | ||
| const exp = parseInt(exponent, 10); | ||
| return exp < 0 ? Math.abs(exp) + coefficientDecimals : coefficientDecimals; | ||
| } | ||
| // Normal decimals | ||
| const decimalPart = str.split('.')[1]; | ||
| return decimalPart ? decimalPart.length : 0; | ||
| } |
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.
Calling numberUtil.getPrecision instead.
|
|
||
| // Fix #21099 | ||
| const precision = numberUtil.getPrecisionSafe(rawVal) || 0; | ||
| let powVal = parseFloat(fixRound(rawVal, precision as number, true)); |
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.
@SihongShen @Ovilia I don't fully understand the purpose of this handling.
If rawVal has the fractional part digits <= 20, rawVal will be equal to powVal, nothing changes.
Otherwise, if the fractional part digits > 20, the number.round method internally clamp it to 20, but that doesn't make sense in this scenario and may lead incorrect results, e.g, (1e-40).toFixed(20) we get 0.
From my understanding, the "fix round" applied in src/scale is meant to correct rounding errors introduced by floating-point computation, (such as the well-known case where 0.1 + 0.2 === 0.30000000000000004, when representing and calculating decimal fractional number in binary).
But Math.pow(base, tick.value) shouldn't suffer from such kind of rounding error, because the Log.ts guarantees interval and tick.value to be integers, and the base is typically an integer as well. They can be exactly represented in ieee754 64-bit floating-point number (assume no overflow - this is an separate topic).
Therefore, there is no opportunity for rounding errors to be accumulated beyond Number.EPSILON * Math.pow(2, ieee754_double_exponent(tick.value)) and result in a different result when converted back to a decimal string.
(Correct me if I'me wrong.)
Therefore, I think we should just remove this precision and rounding handling here. Just be:
let powVal = mathPow(base, val);I tried to fix it in #21120
Brief Information
This pull request is in the type of:
What does this PR do?
Fixes issue #21099, now axis ticks can show number smaller than 1e-10.
Fixed issues
#21099: log axis ticks not containing value less than 1e-10
Details
Before: What was the problem?
No log axis ticks containing value less than 1e-10
After: How does it behave after the fixing?
Log axis ticks smaller then 1e-10 can be shown on the graph.
Document Info
One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information