Support negative values for log Scale. #20872
Conversation
|
Thanks for your contribution! |
Antti-Palola
force-pushed
the
fix-logarithmic-scale-negative-below-one
branch
2 times, most recently
from
c67121f to
17b68f2
Compare
|
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20872@17b68f2 |
Support implemented by inverting the extents at input and handling the input as absolute value and finally mapping everything back to negative axes in inverted order. Added test cases for passing through 0 power and new a new file ogScale-negative.html.
Properly inherit IntervalScale and inline the only thing that really was required from Scale base class `unionExtent`.
Add limit to major tick generation to stop at base extents. If log base 10 range is from 4 to 200, previously ticks overflowed from top and bottom to 1-1000. Create minor ticks within decade linearly spaced as before but if the extent is not within even log steps, stop generating at extent end. When major ticks are more than one decade apart, generate a minor tick for each decade up to the split number times.
Antti-Palola
force-pushed
the
fix-logarithmic-scale-negative-below-one
branch
from
85787f2 to
b6aeb01
Compare
|
@Ovilia Hey, could you take a look at this: an improved shot at fixing log scaled line plots. |
| const scaleProto = Scale.prototype; | ||
| // FIXME:TS refactor: not good to call it directly with `this`? | ||
| const intervalScaleProto = IntervalScale.prototype; | ||
|
|
||
| const roundingErrorFix = numberUtil.round; |
There was a problem hiding this comment.
class Log has been refactored in echarts v6.
Therefore, this PR has some conflicts with the current codebase.
| // If both extent are negative, switch to plotting negative values. | ||
| // If there are only some negative values, they will be plotted incorrectly as positive values. | ||
| this._isNegative = true; | ||
| } |
There was a problem hiding this comment.
A axis scale is commonly shared by multiple series.
unionExtent is called multiple times if there are multiple series using this axis (scale).
Even if there is only one "series", sometimes users might call chart.setOption multiple times with no data.
I'm worried that auto-detect _isNegative here is error-prone and might confuse users if any unexpected behavior occurs.
I think a "all negative log scale" is inherently not self-adaptable - it can be only used in the specific case where all data in series are negative. If we provide that feature, it should be explicitly declared in option - making users know that they're using that feature for that specific case.
See also the comment at the header of this code review.
| * @returns The absolute logarithm value, or 0 if x is very close to 0 | ||
| */ | ||
| export function absMathLog(x: number, base = 10): number { | ||
| if (Math.abs(x) < Number.EPSILON) { |
There was a problem hiding this comment.
I do not fully understand the choice of Number.EPSILON here yet.
- Consider an input
1e-20, smaller thanNumber.EPSILONbut can be represented in a 64-bit float number, andlog10(1e-20)is-20- a normal number. I think it's not reasonable enough to exclude them. log(1)is0;log(near_zero)is supposed to be a big negative number, but return 0 here. It may cause unexpected results in the subsequent calculation?
| * @param splitNumber Get minor ticks number. | ||
| * @returns Minor ticks. | ||
| */ | ||
| getMinorTicks(splitNumber: number): number[][] { |
There was a problem hiding this comment.
This new implementation introduces a breaking change to minor ticks in logarithmic axis.
Previously minor ticks is in log axis is based on the raw value (before log performed), like:
The effect in this PR is based on the value that log is performed:
I thinks both of approaches make some sense (though personally I think the previous one might be more meaningful to hint users the fact of logarithm).
Anyway,
- If we introduce a new effect, the previous should be still preserved. An new option can be introduced to controlled this.
- Consider the maintenance and code size, I think the new effect could reuse the minor tick logic in
scale/Interval.ts, rather than implement in another way.
|
|
||
| const ticks = intervalScaleProto.getTicks.call(this, expandToNicedExtent); | ||
| // Ticks are created using the nice extent, but that can cause the first and last tick to be well outside the extent |
There was a problem hiding this comment.
The span of nice extent is supposed to be greater than (or equals to) the extent calculated from series.data.
In many cases, and by convention, the max/min data value are not expected to be displayed on the edge of the Cartesian. (otherwise, if need that, use option like yAxis.min/max to change the behavior.
We should have both of the behavior provided.
| let interval = mathMax( | ||
| 1, | ||
| mathRound(span / approxTickNum) | ||
| ); |
There was a problem hiding this comment.
The original implementation uses the 10-based value as interval, while this PR do not use that strategy.
The current effect in this PR:
I think if introducing other base (e.g., 2-based ticks), it is a separate topic, and should be applied uniformly rather than only in log scale, and controlled by some new options.
Moreover, this change seems to be inconsistent with the following process like err <= 0.5.
| const scaleProto = Scale.prototype; | ||
| // FIXME:TS refactor: not good to call it directly with `this`? | ||
| const intervalScaleProto = IntervalScale.prototype; | ||
|
|
||
| const roundingErrorFix = numberUtil.round; | ||
|
|
There was a problem hiding this comment.
From my understanding:
A new log scale method should enabled explicitly by a new option
e.g., yAxis.method: 'asinh' | 'symlog' | 'negative' (or some other option names like 'scaleMapping', 'mappingMethod', 'mapping', ...)
Log scale methods comparison
asinh/symlog/signed log1p- I think
asinh/symlog/signed log1pmight be a more thorough or meaningful solution for negative values. This is because an axis is commonly shared by multiple series, and a series might have both positive and negative values.symloghas been available in matplotlib for a long time, whileasinhoffers better mathemetical properties. (feat(chart): support negative values in logarithmic axes #16547 tried to introducesymlog).signed log1p(sign(x) * log(1 + |x|)) is a similar approach but not infinite differentiable in 0.
- I think
Math.log1p- Can also be an optional method, since it's frequently used in scenario like cumulative return factors. It was also mentioned in this comment.
negative log(proposed in this PR)- I think it can only serve for case that all data values of all series are negative (a axis scale serves multiple series). I'm not sure whether this kind of scenario occurs frequently or not.
Additionally, consider value 0
asinh/symlog/signed log1p/Math.log1pinherently handles it.normal log/negative logrequires zero values filtered (in "scale union from series"), if intending to address it. And inlog1pcase, value smaller than -1 is supposed to be filtered.
Some related test cases:
option = {
yAxis: {type: 'log'},
xAxis: {data: ['d1', 'd2', 'd3']},
series: [{
id: 'a',
type: 'line',
data: [123, 456, 789], // all positive, sharing one yAxis
}, {
id: 'b',
type: 'line',
data: [-0.111, -0.333, -20], // all negative, sharing one yAxis
}, {
id: 'c',
type: 'line',
data: [611, -12, 23], // contain positive and negative, sharing one yAxis
}, {
id: 'd',
type: 'line',
data: [98, 0, -12], // contain zero, sharing one yAxis.
}, {
id: 'e',
type: 'bar',
data: [67, 91, 23], // bar start value is 0 by default.
}]
}
After this comparison, I'm afraid the "negative log" method proposed in this PR doesn't seem sufficiently necessary to be a built-in method in echarts. Correct me if I missed any cases that require this method and can not be covered by other methods above.
|
Thank you for the in-depth review 🙏🏻 I won't be able to get back to this right away but will look into it asap. |
Support implemented by inverting the extents at input and handling the input as absolute value and finally mapping everything back to negative axes in inverted order.
Added test cases for passing through 0 power and new a new file logScale-negative.html.
Brief Information
This pull request is in the type of:
What does this PR do?
Add support for plotting negative values on Log scales.
Fixed issues
Fix a previous PR #16547 more generically and correctly.
Details
Before: What was the problem?
Negative values could not be plotted with log scaling.
After: How does it behave after the fixing?
Log scale can now be used
Before: nothing is drawn
After
Document Info
One of the following should be checked.
There is no mention of negative values having problems in docs.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information