Skip to content

prefer lineX when Y is monotonic, lineY when X is monotonic, for better tooltips #1558

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

Merged
merged 3 commits into from
May 15, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented May 15, 2023

This applies apply the same logic as for area to decide between lineX and lineY, but, when not sure, defaults to line (whereas area defaults to areaY).

closes #1547

@Fil Fil requested a review from mbostock May 15, 2023 17:45
…ut when not sure, default to line (whereas area defaults to areaY).

closes #1547
@Fil Fil force-pushed the fil/auto-line-monotonic branch from 00b176f to c26e64d Compare May 15, 2023 17:47
@Fil Fil marked this pull request as draft May 15, 2023 17:51
@Fil
Copy link
Contributor Author

Fil commented May 15, 2023

Sorry this is not ready yet.

Comment on lines 93 to 104
markImpl =
X && Y // same logic as area (see below), but default to line
? yZero
? lineY
: xZero || isMonotonic(Y)
? lineX
: isMonotonic(X)
? lineY
: line
: X // 1d line by index
? lineX
: lineY;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both x and y are monotonic, and neither xZero nor yZero are specified, I think we should default to lineY rather than lineX. If this works, I think I’ll push this:

Suggested change
markImpl =
X && Y // same logic as area (see below), but default to line
? yZero
? lineY
: xZero || isMonotonic(Y)
? lineX
: isMonotonic(X)
? lineY
: line
: X // 1d line by index
? lineX
: lineY;
markImpl =
X && Y // same logic as area (see below), but default to line
? yZero || isMonotonic(X)
? lineY
: xZero || isMonotonic(Y)
? lineX
: line
: X // 1d line by index
? lineX
: lineY;

@mbostock mbostock marked this pull request as ready for review May 15, 2023 17:56
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with the latest commit I pushed, but I can re-review if there’s something else I missed! 🙏

@Fil
Copy link
Contributor Author

Fil commented May 15, 2023

I'm testing it with #1546, and it looks good for lines with two channels.

However I don't like that autoLineHistogram is using lineX (not a new thing); it's specified in the 1d line by index part, and it seems we must reverse lineX and lineY here.

Comment on lines +101 to +102
? lineX
: lineY;
Copy link
Contributor Author

@Fil Fil May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per the above comment:

Suggested change
? lineX
: lineY;
? lineY
: lineX;

i'd also want to add this test

export async function autoLineHistogramY() {
  const aapl = await d3.csv<any>("data/aapl.csv", d3.autoType);
  return Plot.auto(aapl, {y: "Volume", mark: "line"}).plot({marginLeft: 80});
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the problem, but I don’t think that’s the right fix. I think the additional context in this example is that there’s a reducer for y, and so even those only x is provided as input, x is binned, effectively meaning that x is monotonic (and y is derived). So, we need to consider the reducers when determining the mark implementation, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think the latest commit fixes the problem.

@Fil Fil merged commit 801b730 into main May 15, 2023
@Fil Fil deleted the fil/auto-line-monotonic branch May 15, 2023 20:04
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
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.

The auto mark could be smarter and choose lineY
2 participants