Fix log scale calculations #6903
Conversation
|
Can you share the images for |
| @@ -69,19 +62,20 @@ const defaultConfig = { | |||
| class LogarithmicScale extends Scale { | |||
| _parse(raw, index) { // eslint-disable-line no-unused-vars | |||
| const value = LinearScaleBase.prototype._parse.apply(this, arguments); | |||
| return helpers.isFinite(value) && value >= 0 ? value : undefined; | |||
| if (value === 0) { | |||
There was a problem hiding this comment.
I think I prefer calculating minPositive in _getMinMax rather than handling 0 specially here. (Actually probably what would be nicest is overriding _getMinMax in the log scale, but the _getMinMax logic has gotten so complicated that's a bit harder).
E.g. what if someone has a toggle between linear and log scales and wants to set parsing: false? I think the more we can keep scales out of the parsing business the better.
There was a problem hiding this comment.
Data with only positive values (and 0) works fine without parsing, if its in the correct format. One needs to set min value to scale manually then, but that would be expected when parsing=false, wouldn't it?
I have a follow-up to make beginAtZero work nicely on log scale, but I'm getting bored with this.
There was a problem hiding this comment.
In my mind parsing is just that. It doesn't include validation or anything else. But I don't necessarily want to spend more time on this, so if you and @etimberg are both okay with this then so am I
| if (value > me.min && value > 0) { | ||
| decimal = (log10(value) - me._startValue) / me._valueRange + me._valueOffset; | ||
| if (value === undefined || value === 0) { | ||
| value = me.min; |
There was a problem hiding this comment.
can we return null here or something? this seems a little funny and I'm not quite sure why we're doing it
There was a problem hiding this comment.
Returning min value of scale for 0 (and parsed 0 that will be undefined), so those values are drawn.
There was a problem hiding this comment.
Why use the scale min instead of undefined though? It seems strange to me to change it to a somewhat arbitrary different value rather than making it undefined
There was a problem hiding this comment.
Lets make one thing clear, do you think we should continue to draw zero on a log chart or not?
There was a problem hiding this comment.
I would say we should not
There was a problem hiding this comment.
There are few fixed issues about that. #4913 fixed some for v2.7.1
If its not clear already, this is not something I want to spend my time with.
There was a problem hiding this comment.
Ok. I'm not sure how this will work in all cases, but if we need to change it later I suppose we can
Just lowering the min value so 0 and the lowest non zero are differentiated. If scale breaks would be implemented, then we could add a break in between. |
At first this sounded funny to me, but I after thinking about it for awhile I think this is actually a pretty good default for drawing bar charts on a log scale. I did a google image search for bar chart log scale and lots of them were drawn this way |
|
We should find a way to keep that functionality, though I agree that it is out of scope for this PR |
My suggestion in addition to #6902
Fixes: #6934