OHLC equal open close cases fix #1655
Conversation
src/traces/ohlc/helpers.js
Outdated
| switch(direction) { | ||
| case 'increasing': | ||
| return function(o, c) { return o <= c; }; | ||
| fn = function(o, c) { | ||
| if(o === c) { |
There was a problem hiding this comment.
We haven't cast to numbers, have we? Looks like this will break if we mix strings and numbers (or use different string representations of the same number, like '2.0' and '2.00'
I guess a point should get dropped if any of o, h, l, c is not numeric?
There was a problem hiding this comment.
checked all to ensure they're numeric in 597adf9
src/traces/ohlc/helpers.js
Outdated
| } | ||
| } | ||
| return o > c; | ||
| }; |
There was a problem hiding this comment.
This is a lot of duplication with just a few true <-> false flips... looks like we could keep the perf up without the duplication if we made one function isIncreasing(o, c), which looks at a state var isPrevIncreasing (which would also simplify the logic there as we wouldn't need to deal with null, just initialize it to true), then move the switch logic into creating the wrapper function below:
function increasingFilter(o, c) {
var isThisIncreasing = isIncreasing(o, c);
isPrevIncreasing = isThisIncreasing; // why add !! to this?
cPrev = c;
return isThisIncreasing;
}
function decreasingFilter(o, c) {
var isThisIncreasing = isIncreasing(o, c);
isPrevIncreasing = isThisIncreasing; // why add !! to this?
cPrev = c;
return !isThisIncreasing; // the only difference...
}
There was a problem hiding this comment.
@alexcjohnson I vaguely remember you mentioning on slack that you had a commit implementing your solution on a branch. If that's not case, I'll try to push something up to get this PR in this week's 1.28.0.
There was a problem hiding this comment.
simplified these filters (even more than ^^) in 9575ab6
|
hah, the test failure is a real bug that's fixed by 597adf9 |
Nice 🎉 |
| } | ||
|
|
||
| function isDecreasing(o, c) { | ||
| return isNumeric(o) && isNumeric(c) && !_isIncreasing(+o, +c); |
There was a problem hiding this comment.
that !_isIncreasing statement is so brilliant ✨
|
💃 Thanks very much for taking this one to the finish line 🏎️ 🏁 |
|
Haven't had a chance to look into it all, but thank you for seeing it all through 👍 |
supersedes @noway's #1619 with tests and lint fixes.
@noway does the test case look ok to you?