Skip to content

Conversation

@Koookadooo
Copy link

@Koookadooo Koookadooo commented Oct 13, 2025

Fixes #7581
Supersedes plotly/plotly.js#6490

Summary

Replace cumulative t += step loop in src/traces/violin/calc.js with 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:

  • Underrun: stop early and leave the tail of cdi.density unfilled → Cannot read properties of undefined (reading 'v') (often seen via Kaleido).
  • Overrun: iterate once too many → severe slowness / TimeoutError during export.

This PR computes t from the integer index on each iteration so density.length === n deterministically and avoids cumulative error.

Change (logic only)

- for (var k = 0, t = span[0]; t < (span[1] + step / 2); k++, t += step) {
+ for (var k = 0; k < n; k++) {
+     var t = span[0] + k * step;
      var v = kde(t);
      cdi.density[k] = { v: v, t: t };
      maxKDE = Math.max(maxKDE, v);
}

Why it works:

We already compute n = Math.ceil(dist / (bandwidth/3)) and step = dist / n. Index-based sampling guarantees exactly n points 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:

it('should produce exactly n density samples for tiny or near-equal spans', function() {
    var cd = _calc({
        type: 'violin',
        x: [0, 0],
        y: [0.5006312999999999, 0.5006313]
    });
    var cdi = cd[0];

    var dist = cdi.span[1] - cdi.span[0];
    var n = Math.ceil(dist / (cdi.bandwidth / 3));

    expect(cdi.density.length).toBe(n);
});

verified locally with:

npm run lint
npm run test-jasmine -- violin

Demo

  • CodePen comparison (old vs fixed):

https://codepen.io/Koookadooo/pen/PwZKrrv?editors=1111

  • Local file for download to run manually:

calc_test.js

run with:

node calc_test.js

debug_html.py

to run, build my branch locally with:

npm run build

or

npm run bundle

and then run:

python debug_html.py --plotly-js "path\to\local\build\plotly.js" --open

Impact

  • Fixes Cannot read properties of undefined (reading 'v') and TimeoutError in KDE sampling.
  • No schema or layout changes.
  • Performance neutral or slightly improved.

@Koookadooo
Copy link
Author

Koookadooo commented Oct 14, 2025

Not sure why diff images in tests are failing I checked these against plotly's test_dashboard using the following:

npm install
npm run pretest
npm run start

Original Code:

image

violin_non-linear image in test_dashboard:

image

Change in code to ensure test_dashboard is picking up my changes (n = 3):

image

Tabs.reload()

violin_non-linear image in test_dashboard:

image

My Proposed Fix:

image

Tabs.reload()

violin_non-linear image in test_dashboard:

image

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.

@emilykl
Copy link
Contributor

emilykl commented Oct 22, 2025

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 test-baselines CI job renders each test plot according to the updated code, and compares the resulting image to the "ground truth" images stored in test/image/baselines/.

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 master you can use the tasks/circleci_image_artifact_download.sh script @camdecoster added to do that.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Violin calc density loop fails with tiny/near-equal spans - underrun ('undefined.v') or overrun (timeout)

2 participants