fix(violin): prevent FP drift in KDE sampling loop (avoid underrun/overrun on tiny spans) #7582
Conversation
…errun on tiny spans)
|
Not sure why diff images in tests are failing I checked these against plotly's test_dashboard using the following: Original Code: 
violin_non-linear image in test_dashboard: 
Change in code to ensure test_dashboard is picking up my changes (n = 3): 
violin_non-linear image in test_dashboard: 
My Proposed Fix: 
violin_non-linear image in test_dashboard: 
Unless all of the diff images just show the difference in the pixels between the base and the new plot renders. In that case, all of the differences appear at the ends of the violin shapes, which makes sense — the old loop accumulated floating-point rounding error, so the violin tips were drawn slightly differently each time. With the new index-based sampling, the KDE grid is consistent and removes that drift, which explains these small end-cap differences. |
@Koookadooo Yes, this is correct. The So when making a change that affects how a plot is displayed visually, it's normal and appropriate that some of the image tests will fail, and what it it means is that the baseline images need to be updated. Once you've convinced yourself that the changes are expected, you can download the updated images as artifacts from the CI job and commit them. If you rebase off of FWIW I've pushed a commit on a separate branch here with the new images so that I could use GitHub's diff interface to see the changes easily. It looks like in general the effect is that some of the ends of the violins are chopped shorter. Is that expected? |
…dpoints as samples
…rvals means n + 1 samples.
|
@emilykl No, the chopped ends were not expected. The first change ended up sampling to span[1] - step (dist /n) which ended up leaving off the end point. Upon further inspection, the cdi.density was being set to n where n is the number of intervals (Math.ceil(dist / (bandwidth/3))). With step = dist / n there are n intervals meaning we have n+1 interval points, so to deterministically include both span endpoints we must sample n+1 points. The original t-based loop (t += step with t < span[1] + step/2) implicitly allowed that endpoint sample but relied on incremental addition, which accumulated floating-point drift and made iteration counts non-deterministic. Changing to index-based sampling and allocating cdi.density = new Array(n + 1) lets us compute each t as span[0] + k*step (no accumulation), so we deterministically include the endpoint and avoid drift. This approach ended up only producing 1 test baseline image failure and it failed by 3 pixels. It also fixes the original floating point accumulation issue that caused the timeout or undefined errors. |
|
@Koookadooo Thanks for the thorough explanation, that makes sense. It's not surprising to me that one of the violin baselines would change imperceptibly as a result of these changes, so that's fine as well. Does that mean these changes are ready for another review? I'll take a final look through within the next day or two. |
|
@emilykl Yes, ready for review. Cheers! Also, thanks for committing the new images and linking the diff, that made it easy to see what was happening. |
There was a problem hiding this comment.
@Koookadooo Two very minor suggestions (one is just draftlog formatting) otherwise LGTM! Thanks for your work on this.
|
nice fix 👏 |
Fixes #7581
Supersedes plotly/plotly.js#6490
Summary
Replace cumulative
t += steploop insrc/traces/violin/calc.jswith an index-based loop to eliminate floating-point drift when spans are extremely small (≈1e-13).With near-equal, high-precision values, the KDE sampling loop accumulates FP error via
t += step. Depending on drift direction this can:cdi.densityunfilled →Cannot read properties of undefined (reading 'v')(often seen via Kaleido).TimeoutErrorduring export.This PR computes
tfrom the integer index on each iteration sodensity.length === ndeterministically and avoids cumulative error.Change (logic only)
Why it works:
We already compute
n = Math.ceil(dist / (bandwidth/3))andstep = dist / n. Index-based sampling guarantees exactlynpoints with no cumulative drift, fixes both failure modes, and preserves existing spacing and scaling.Tests
Added a jasmine test in
test/jasmine/tests/violin_test.js:verified locally with:
Demo
https://codepen.io/Koookadooo/pen/PwZKrrv?editors=1111
calc_test.js
run with:
debug_html.py
to run, build my branch locally with:
or
and then run:
Impact
Cannot read properties of undefined (reading 'v')andTimeoutErrorin KDE sampling.