Skip to content

[BUG]: sankey: node.x / node.y are normalized against the wrong extent when orientation: 'v' #7947

Description

@wf-r

sankey: node.x / node.y are normalized against the wrong extent when orientation: 'v'

Summary

For vertical Sankey traces, user supplied node.x / node.y are scaled by the
un-transposed plotting area, while the layout they are written into is
transposed. On a non-square plotting area the documented range of [0, 1] is
therefore neither honoured on input nor produced on output: nodes are placed
outside the plotting area, and the values that Plotly itself writes back after
a drag can exceed 1.

Where it happens

src/traces/sankey/render.js, in sankeyModel():

var width  = layout.width  * (domain.x[1] - domain.x[0]);
var height = layout.height * (domain.y[1] - domain.y[0]);
...
sankey.size(horizontal ? [width, height] : [height, width]);   // (1) transposed
...
var pos = [trace.node.x[i] * width, trace.node.y[i] * height]; // (2) not transposed

The d3-sankey layout space always carries the flow along its x axis. For
orientation: 'v' that space is transposed onto the screen by the group
transform, so at (1) its extent is correctly given as [height, width]. At (2)
the user coordinates are nevertheless scaled by width / height, i.e. by the
extents of the other axes.

The write-back path has the same asymmetry, in
persistFinalNodePositions():

x.push(nodeX / d.figure.width);
y.push(nodeY / d.figure.height);

Consequences

With a plotting area of width × height and orientation: 'v', the usable
input range becomes

  • node.x[0, height / width]
  • node.y[0, width / height]

and the values emitted by plotly_restyle after a node drag land in the same
ranges. Concretely, on a 900 × 300 area (aspect ratio 3) the flow axis is
stretched by a factor of 3 and the cross axis compressed by the same factor:

node node.x node.y layout x (actual → intended) layout y (actual → intended)
A 0.10 0.25 90 → 30 75 → 225
B 0.40 0.75 360 → 120 225 → 675
C 0.70 0.50 630 → 210 150 → 450
D 0.95 0.50 855 → 285 150 → 450

The layout x extent is only 300, so B, C and D are pushed off the canvas
along the flow axis while everything is squeezed against the cross-axis edge.

The bug is invisible on a square plotting area and invisible for
orientation: 'h', which is presumably why it has survived.

Steps to reproduce

Render two traces with identical node.x / node.y on a non-square area, one
with orientation: 'h' and one with orientation: 'v':

var nodeX = [0.10, 0.40, 0.70, 0.95];
var nodeY = [0.25, 0.75, 0.50, 0.50];

Plotly.newPlot(gd, [{
  type: 'sankey',
  orientation: 'v',            // 'h' for the working reference
  arrangement: 'freeform',
  node: {label: ['A','B','C','D'], x: nodeX, y: nodeY, pad: 10, thickness: 18},
  link: {source: [0,1,2], target: [2,2,3], value: [4,2,6]}
}], {width: 900, height: 300, margin: {l: 0, r: 0, t: 0, b: 0}});

Horizontal rendering:

Image

Vertical rendering

Image

Expected: all four nodes inside the plotting area in both cases, since every
coordinate is within (0, 1).

Actual: the horizontal trace is fine; in the vertical trace three of the
four nodes are clipped. Dragging a node in the vertical trace and listening to
plotly_restyle yields node.y values above 1.

Two possible fixes

Both are small and mutually exclusive. They differ in what node.x is
defined to mean, so the choice is an API decision rather than an
implementation detail.

Option A — normalize against the layout extent

Keep the current semantics (node.x is the position along the flow axis)
and fix only the scale factor:

var layoutWidth  = horizontal ? width : height;
var layoutHeight = horizontal ? height : width;
...
var pos = [trace.node.x[i] * layoutWidth, trace.node.y[i] * layoutHeight];

with the matching inverse in persistFinalNodePositions().

  • Pro: minimal diff; semantics of node.x stay orientation-independent in
    the graph-theoretic sense (it always addresses the layer axis), so a set of
    coordinates keeps its meaning when the orientation is toggled.
  • Con: contradicts the documented wording "the normalized horizontal
    position"
    , which for a vertical diagram is then simply false. Users who read
    positions off the screen have to mentally transpose.
  • Breakage: vertical traces with explicit node.x / node.y on a
    non-square area. Square areas and horizontal traces are unaffected.

Option B — normalize against the screen

Redefine the attributes to mean what they say: node.x is always horizontal on
screen, node.y always vertical, and convert on the way in and out.

function userToLayout(horizontal, width, height, px, py) {
    return horizontal ? [px * width, py * height] : [py * height, px * width];
}

function layoutToUser(horizontal, width, height, lx, ly) {
    return horizontal ? [lx / width, ly / height] : [ly / width, lx / height];
}

Because the layout extent for v is [height, width], the factors cancel and
both values are guaranteed to stay in [0, 1] for every orientation.

  • Pro: matches the existing attribute descriptions and matches the mental
    model of someone looking at the rendered figure; no new model fields; the
    same coordinates keep a node in the same place on screen when the orientation
    is toggled.
  • Con: the meaning of node.x now depends on orientation — for a
    vertical diagram it addresses the cross axis, so callers who think in layers
    have to swap the two arrays when switching orientation.
  • Breakage: larger than Option A — every vertical trace with explicit
    coordinates changes, including on square plotting areas.

Open questions

  1. Which of the two semantics should node.x / node.y carry? The attribute
    descriptions currently imply B, the implementation attempted A.
  2. Should the change be gated behind a new attribute (e.g. a coordinate-space
    flag) to avoid breaking existing figures, or is a straight fix acceptable
    given that the current behaviour is unusable on non-square areas anyway?

Suggested target

Either option is a breaking change for existing figures, so this should land in
v4.0. That also lets the coordinate conversion be worked out together with the
direction attribute in one go: the reversed case adds a further mirroring in
sankeyTransform that has to be folded into the same conversion, and doing it
in two separate passes would mean touching the same lines twice.

I am happy to open a PR for whichever option the maintainers prefer.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugsomething broken

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions