-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
remove cached trace._interpz
from heatmaps/contour maps with gaps
#2657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
it turned out to be more trouble to properly invalidate this cache than it was worth - especially for edge cases like removing a previously filled-in value.
Good call, just to list a few things high on my priority list:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix and tests 💃
Would you mind sharing some benchmarks for interp2d
? Mostly to document how much of a slow down users can expect in the next release in case I don't have time to resolve #2546 by then.
/* | ||
* interp2d: Fill in missing data from a 2D array using an iterative | ||
* poisson equation solver with zero-derivative BC at edges. | ||
* Amazingly, this just amounts to repeatedly averaging all the existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazingly ✨
return Plotly.react(gd, [{ | ||
type: 'contour', | ||
// notice that this one is the same as the previous, except that | ||
// a previously present value was removed... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this comment 📚
Benchmarks - compare a full heatmap, which plots in around 70ms for me at n=100 (100x100 heatmap): function hmFull(n) {
var z = new Array(n); for(var i = 0; i < n; i++) { z[i] = new Array(n); for(var j = 0; j < n; j++) z[i][j] = Math.random(); }
var t0 = performance.now();
Plotly.newPlot(gd, [{z: z, type: 'heatmap', showscale: false}], {width: 500, height: 500})
return performance.now() - t0;
} to a gapped one: function hmPartial(n) {
var z = new Array(n); for(var i = 0; i < n; i++) {
z[i] = new Array(n);
for(var j = 0; j < n; j++) z[i][j] = null; z[i][Math.floor(Math.random() * n)] = Math.random();
}
var t0 = performance.now();
Plotly.newPlot(gd, [{z: z, type: 'heatmap', showscale: false, connectgaps: true}], {width: 500, height: 500})
return performance.now() - t0;
} A fairly full heatmap (n=100, m=50, so about 50% gaps) spends about 40ms on |
Fixes #2652 - turns out this isn't a react-specific issue, it's just about changing z data for contours with gaps in the data (and heatmaps with
connectgaps: true
). We were caching the interpolation results, even across recalc calls, because this can be a pretty slow (iterative) process. I removed that cache, after first trying to figure out whether we could salvage it, based on comparing the incoming known (non-interpolated) z values to the old ones. It turned out that was difficult (and expensive) to get right, especially for edge cases like removing a previously filled-in value without changing anything else.We're doing a better job than we used to do (3+ years ago when this cache was added) at avoiding unnecessary recalcs, so I don't think this should be too big a cost, but this is more motivation to push in that direction, especially things like recalc'ing only the traces that need it.
cc @etpinard