fix(sankey): derive effective node.pad clamp from layout geometry - #7977
Open
CAOShurong wants to merge 9 commits into
Open
fix(sankey): derive effective node.pad clamp from layout geometry#7977CAOShurong wants to merge 9 commits into
CAOShurong wants to merge 9 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
The node.pad warning read the clamped padding back through sankey.nodePadding(). In @plotly/d3-sankey@0.7.x that getter returns the post-layout clamped value, but since 0.12.x (upstream split of dy/py) it returns the configured value, so the comparison is never true and the warning never fires after a dependency upgrade. Measure the smallest vertical gap between consecutive nodes in any one column instead; this works regardless of the installed d3-sankey version. Regression tests cover both the clamped and non-clamped paths. Refs plotly#7832
The inner loop reused the file-scope `i` before its var declaration line, tripping biome's noInvalidUseBeforeDeclaration, and the PR was missing its changelog draftlog entry.
The karma viewport size does not determine the plot size, so the small-figure case rendered at default dimensions and never clamped. Set explicit layout width/height (and small margins) instead, and use the standard createGraphDiv() helper.
A 24-node chain places one node per column, so no column ever holds two nodes and the padding can never clamp regardless of figure size. A one-to-many star puts all 24 sinks in one column (verified against the real d3-sankey layout: effective min gap 2.32 at 480x80).
Lib.warn is variadic; pushing only the first argument made warnings[0][0] a single character instead of the message prefix.
At 24 sink nodes a 900-high figure still clamps the padding (effective gap 26.7 < 30); 880 was verified against the real layout to leave enough room. Also fix the leftover single-arg spy in this test.
With 24 sink nodes the padding only stops clamping once the plot area is ~1000px high (verified against the real layout: gap 27.3 at 900, exactly 30 at 1000).
CAOShurong
force-pushed
the
codex/sankey-7832-effective-padding
branch
from
7a8dae8 to
43b5579
Compare
August 25, 2026 09:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #7832.
The node.pad reduction warning in src/traces/sankey/render.js read the clamped padding back through sankey.nodePadding(). In @plotly/d3-sankey@0.7.x that getter returns the post-layout clamped value, but since 0.12.x (the upgrade prepared in #7830, where upstream split internal dy from py) it returns the user-configured value instead. After such an upgrade the comparison sankey.nodePadding() < nodePad is never true and the warning silently stops firing — exactly the regression #7832 describes.
This PR derives the effective padding from the laid-out node geometry instead: it measures the smallest vertical gap between consecutive nodes within any single column of the computed sankey graph. This works identically regardless of which @plotly/d3-sankey version is installed, so the diagnostic survives future dependency upgrades.
Verification
I reproduced the getter-semantics difference directly against both dependency versions (outside plotly.js), with a 25+25-node two-column layout, requested nodePadding(30) and extent height 50:
The real layout clamps identically in both versions; only the getter changed. The new geometry-based check reports ~1.389 on both.
Two jasmine regression tests are added to test/jasmine/tests/sankey_test.js:
Note: I could not run the full karma suite in my sandbox (headless Chrome disconnects mid-suite, an environment limitation unrelated to this change); node --check passes on both modified files and the change is confined to the warning path after sankey() completes. CI here will exercise the full suite.