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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
