You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by "Antti-Palola (via GitHub)" <gi...@apache.org> on 2025/03/27 21:10:48 UTC
[PR] Support negative values for log Scale. [echarts]
Antti-Palola opened a new pull request, #20872:
URL: https://github.com/apache/echarts/pull/20872
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.
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [] bug fixing
- [x] new feature
- [ ] others
### What does this PR do?
<!-- USE ONE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
Add support for plotting negative values on Log scales.
### Fixed issues
- Sort of resolves: https://github.com/apache/echarts/issues/15558 but will not allow plotting both positive and negative on same axis. A split axis would be needed. One could also argue that for most cases you should never plot negative and positive on same axis as it skews the scales, although it could be done maybe when smallest exponent is 0.
- Resolves: https://github.com/apache/echarts/issues/17459.
Fix a previous PR https://github.com/apache/echarts/pull/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.
- [x] This PR doesn't relate to document changes
- [ ] The document should be updated later
- [ ] The document changes have been made in apache/echarts-doc#xxx
There is no mention of negative values having problems in docs.
## Misc
### ZRender Changes
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
### Related test cases or examples to use the new APIs
N.A.
## Others
### Merging options
- [ ] Please squash the commits into a single one when merging.
### Other information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
Re: [PR] Support negative values for log Scale. [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20872:
URL: https://github.com/apache/echarts/pull/20872#discussion_r2227989001
##########
src/scale/Log.ts:
##########
@@ -27,48 +27,66 @@ import IntervalScale from './Interval';
import SeriesData from '../data/SeriesData';
import { DimensionName, ScaleTick } from '../util/types';
-const scaleProto = Scale.prototype;
-// FIXME:TS refactor: not good to call it directly with `this`?
-const intervalScaleProto = IntervalScale.prototype;
-
const roundingErrorFix = numberUtil.round;
Review 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 log1p` might 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. `symlog` has been available in matplotlib for a long time, while `asinh` offers better mathemetical properties. (#16547 tried to introduce `symlog`). `signed log1p` (`sign(x) * log(1 + |x|)`) is a similar approach but not infinite differentiable in 0.
+ `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](https://github.com/apache/echarts/issues/15558#issuecomment-904022244).
+ `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.log1p` inherently handles it.
+ `normal log` / `negative log` requires zero values filtered (in "scale union from series"), if intending to address it. And in `log1p` case, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
Re: [PR] Support negative values for log Scale. [echarts]
Posted by "100pah (via GitHub)" <gi...@apache.org>.
100pah commented on code in PR #20872:
URL: https://github.com/apache/echarts/pull/20872#discussion_r2218700157
##########
src/scale/Log.ts:
##########
@@ -27,48 +27,66 @@ import IntervalScale from './Interval';
import SeriesData from '../data/SeriesData';
import { DimensionName, ScaleTick } from '../util/types';
-const scaleProto = Scale.prototype;
-// FIXME:TS refactor: not good to call it directly with `this`?
-const intervalScaleProto = IntervalScale.prototype;
-
const roundingErrorFix = numberUtil.round;
Review Comment:
`class Log` has been refactored in echarts v6.
Therefore, this PR has some conflicts with the current codebase.
##########
src/scale/helper.ts:
##########
@@ -136,3 +138,20 @@ export function normalize(val: number, extent: [number, number]): number {
export function scale(val: number, extent: [number, number]): number {
return val * (extent[1] - extent[0]) + extent[0];
}
+
+/**
+ * Calculates the absolute logarithm of a number with a specified base.
+ * Handles edge cases by:
+ * - Returning 0 for values very close to 0 (within Number.EPSILON)
+ * - Taking the absolute value of the input to handle negative numbers
+ *
+ * @param x - The number to calculate the logarithm of
+ * @param base - The base of the logarithm (defaults to 10)
+ * @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) {
Review Comment:
I do not fully understand the choice of `Number.EPSILON` here yet.
- Consider an input `1e-20`, smaller than `Number.EPSILON` but can be represented in a 64-bit float number, and `log10(1e-20)` is `-20` - a normal number. I think it's not reasonable enough to exclude them.
- `log(1)` is `0`; `log(near_zero)` is supposed to be a big negative number, but return 0 here. It may cause unexpected results in the subsequent calculation?
##########
src/scale/Log.ts:
##########
@@ -113,10 +188,19 @@ class LogScale extends Scale {
unionExtent(extent: [number, number]): void {
this._originalScale.unionExtent(extent);
- const base = this.base;
- extent[0] = mathLog(extent[0]) / mathLog(base);
- extent[1] = mathLog(extent[1]) / mathLog(base);
- scaleProto.unionExtent.call(this, extent);
+ if (extent[0] < 0 && extent[1] < 0) {
+ // 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;
+ }
Review 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.
##########
src/scale/Log.ts:
##########
@@ -27,48 +27,66 @@ import IntervalScale from './Interval';
import SeriesData from '../data/SeriesData';
import { DimensionName, ScaleTick } from '../util/types';
-const scaleProto = Scale.prototype;
-// FIXME:TS refactor: not good to call it directly with `this`?
-const intervalScaleProto = IntervalScale.prototype;
-
const roundingErrorFix = numberUtil.round;
const mathFloor = Math.floor;
const mathCeil = Math.ceil;
const mathPow = Math.pow;
-
-const mathLog = Math.log;
-
-class LogScale extends Scale {
+const mathMax = Math.max;
+const mathRound = Math.round;
+
+// LogScale does not have any specific settings
+type LogScaleSetting = {};
+
+/**
+ * LogScale is a scale that maps values to a logarithmic range.
+ *
+ * Support for negative values is implemented by inverting the extents and first handling values as absolute values.
+ * Then in tick generation, the tick values are multiplied by -1 back to the original values and the normalize function
+ * uses a reverse extent to get the correct negative values in plot with smaller values at the top of Y axis.
+ */
+class LogScale extends IntervalScale<LogScaleSetting> {
static type = 'log';
readonly type = 'log';
base = 10;
private _originalScale: IntervalScale = new IntervalScale();
+ /**
+ * Whether the original input values are negative.
+ *
+ * @type {boolean}
+ * @private
+ */
+ private _isNegative: boolean = false;
+
private _fixMin: boolean;
private _fixMax: boolean;
- // FIXME:TS actually used by `IntervalScale`
- private _interval: number = 0;
- // FIXME:TS actually used by `IntervalScale`
- private _niceExtent: [number, number];
-
-
/**
* @param Whether expand the ticks to niced extent.
*/
getTicks(expandToNicedExtent?: boolean): ScaleTick[] {
const originalScale = this._originalScale;
const extent = this._extent;
const originalExtent = originalScale.getExtent();
+ const negativeMultiplier = this._isNegative ? -1 : 1;
+
+ const ticks = super.getTicks(expandToNicedExtent);
+
- 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
Review 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.
##########
src/scale/Log.ts:
##########
@@ -131,13 +215,18 @@ class LogScale extends Scale {
*/
calcNiceTicks(approxTickNum: number): void {
approxTickNum = approxTickNum || 10;
- const extent = this._extent;
- const span = extent[1] - extent[0];
+
+ const span = this._extent[1] - this._extent[0];
+
if (span === Infinity || span <= 0) {
return;
}
- let interval = numberUtil.quantity(span);
+ let interval = mathMax(
+ 1,
+ mathRound(span / approxTickNum)
+ );
Review 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:
<img width="86" height="246" alt="image" src="https://github.com/user-attachments/assets/73c85814-dff7-42c7-8be2-e16db92f85f4" />
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`.
##########
src/scale/Log.ts:
##########
@@ -79,27 +97,84 @@ class LogScale extends Scale {
: powVal;
return {
- value: powVal
+ value: powVal * negativeMultiplier
};
}, this);
}
+ /**
+ * Get minor ticks for log scale. Ticks are generated based on a decade so that 5 splits
+ * between 1 and 10 would be 2, 4, 6, 8 and
+ * between 5 and 10 would be 6, 8.
+ * @param splitNumber Get minor ticks number.
+ * @returns Minor ticks.
+ */
+ getMinorTicks(splitNumber: number): number[][] {
Review 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:
<img width="295" height="264" alt="image" src="https://github.com/user-attachments/assets/4168c656-20b4-4715-986a-09eb2db98b33" />
The effect in this PR is based on the value that log is performed:
<img width="222" height="217" alt="image" src="https://github.com/user-attachments/assets/f19c2486-2e6b-4dbf-a6c4-8e73272f06c3" />
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.
##########
src/scale/Log.ts:
##########
@@ -27,48 +27,66 @@ import IntervalScale from './Interval';
import SeriesData from '../data/SeriesData';
import { DimensionName, ScaleTick } from '../util/types';
-const scaleProto = Scale.prototype;
-// FIXME:TS refactor: not good to call it directly with `this`?
-const intervalScaleProto = IntervalScale.prototype;
-
const roundingErrorFix = numberUtil.round;
Review 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 log1p` might 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. `symlog` has been available in matplotlib for a long time, while `asinh` offers better mathemetical properties. (#16547 tried to introduce `symlog`). `signed log1p` (`sign(x) * log(1 + |x|)`) is a similar approach but not infinite derivative in 0.
+ `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](https://github.com/apache/echarts/issues/15558#issuecomment-904022244).
+ `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.log1p` inherently handles it.
+ `normal log` / `negative log` requires zero values filtered (in "scale union from series"), if intending to address it. And in `log1p` case, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
Re: [PR] Support negative values for log Scale. [echarts]
Posted by "Antti-Palola (via GitHub)" <gi...@apache.org>.
Antti-Palola commented on PR #20872:
URL: https://github.com/apache/echarts/pull/20872#issuecomment-3139754042
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
Re: [PR] Support negative values for log Scale. [echarts]
Posted by "Antti-Palola (via GitHub)" <gi...@apache.org>.
Antti-Palola commented on PR #20872:
URL: https://github.com/apache/echarts/pull/20872#issuecomment-2876443226
@Ovilia Hey, could you take a look at this: an improved shot at fixing log scaled line plots.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
Re: [PR] Support negative values for log Scale. [echarts]
Posted by "echarts-bot[bot] (via GitHub)" <gi...@apache.org>.
echarts-bot[bot] commented on PR #20872:
URL: https://github.com/apache/echarts/pull/20872#issuecomment-2759484936
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
Re: [PR] Support negative values for log Scale. [echarts]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20872:
URL: https://github.com/apache/echarts/pull/20872#issuecomment-2760721411
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20872@17b68f2
<!-- ECHARTS_PR_PREVIEW -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org