Bar with non-numeric (x,y) items bug fix #1519
Conversation
- as `NaN === NaN // => false`
- so that item with non-numeric (x,y) are excluded from the calcdata.
etpinard
changed the title
src/traces/bar/calc.js
Outdated
| cd.push({ p: p, s: s }); | ||
| } else { | ||
| cd.push({ p: p }); | ||
| } |
There was a problem hiding this comment.
Nice. Do we need to merge in the base loop below too?
There was a problem hiding this comment.
Maybe? The behavior now is similar to how scatter fills in its arrayOk attributes. I can double check though.
There was a problem hiding this comment.
Looks like scatter keeps a 1:1 mapping between calcdata points and the input arrays, filling in non-numerics with {x: false, y: false} - that's a good point about other arrayOK attributes, maybe it would be easier to keep that 1:1 mapping for bars too? Should be faster too, as we're not doing repeated .push, can set the size of cd at the start.
There was a problem hiding this comment.
I prefer removing non-numeric items in the calc step, so that we don't have to do that during the plot step (e.g. on zoom).
There was a problem hiding this comment.
OK, but then how do you want to solve this for bar color arrays etc? save the original index in each cd element? cd.push({p: p, s: s, i: i}) or something? That would need to be propagated all the way through to Lib.mergeArray which seems like a bit of a disaster.
Non-numerics should be regarded as an edge case, so the overhead of managing their objects shouldn't be seen as a concern, but you're right to worry about the overhead of checking each valid point. That said, this works for scatter which can have way more points than bars, so I think as long as we make this test super simple it shouldn't be a problem.
There was a problem hiding this comment.
@alexcjohnson ok, you convinced me.
With commit 0144f06 bar calcdata has now a 1-1 correspondence with the bar (x,y) coordinates. Note that non-numeric bar items are cleaned up in bar/plot.js and makeCalcdata (called above the serieslen loop in bar/calc.js) sets non-numeric values to BADNUM.
This a pretty big change that might lead to regressions, so this PR will make be part of the next minor release v1.26.0 only.
- as per AJ's recommendation - and skip over them in setPositions - use BADNUM constant to skip over non-numeric items (recall: non-numeric values are set to BADNUM in makeCalcdata)
- see #1369 for why that was needed.
| var cd = gd.calcdata; | ||
| assertPointField(cd, 'x', [[0, 1, 2, 3]]); | ||
| assertPointField(cd, 'y', [[1, NaN, NaN, 15]]); | ||
| }); |
There was a problem hiding this comment.
do we have a test with a bad x but good y for the same point?
There was a problem hiding this comment.
Good call. Added in 6f0a95d
|
Nice - big improvement. At some point we may want to make sure we have tests that throw some garbage or partially garbage points into ALL our trace types - perhaps just add some junk to various image test mocks that shouldn't impact the images - but it's great that we've locked this down for bars now. 💃 |
Currently
generates
which is wrong.
With this PR, we get