Skip to content

fix misaligned range #1633

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 2 commits into from
May 25, 2023
Merged

fix misaligned range #1633

merged 2 commits into from
May 25, 2023

Conversation

mbostock
Copy link
Member

Fixes #1632.

@mbostock mbostock requested a review from Fil May 24, 2023 23:23
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the rationale. I see three possibilities:

  • We consider it as a user mistake; in that case, we should add a warning, instead of/in addition to truncating.
  • It is expected that users might pass mismatching domain+range in some legitimate cases (which ones?). Then again, why truncate it: it's only the legend that would need to be fixed.
  • It's a missed opportunity to define a color ramp for a quantitative color scale with a simple [min, max] domain.

@mbostock
Copy link
Member Author

The rationale is that the legend is inconsistent with the chart! They should be consistent or else the legend is misleading.

It’s already the documented behavior with D3 that the shorter of the domain and range is used, so I’m just adopting the same behavior in Plot for the legend—we already behaved this way in the plot and scale itself.

I can add a warning, but I don’t think it really merits it because the legend does a better job of explaining what is happening. There are surely other places where we ignore extra arguments and we don’t warn for those.

@mbostock
Copy link
Member Author

To elaborate:

  • I don’t want to change the current scale behavior (in the plot), which I consider to be correct; this PR only changes the legend. Changing the appearance of the legend to match the plot is more conservative than the reverse. I am willing to change the behavior of a plot if we think it’s a bug, but as I said, I think the scale is functioning correctly here so I don’t want to change the behavior. We favor backwards compatibility and stability.

  • The exposed scale included extra (unused) elements in the domain and range. These did not affect the behavior of the scale because the scale ignores these extra elements, but I think it’s clearer to show that they are ignored by truncating the domain or range to match the scale’s behavior.

  • The challenge with shorthand color ramps is: what happens if the domain has more than two elements, too? The current requirement is that the domain and range are parallel. Say we allow the range to contain an arbitrary number of elements if the domain contains two elements. What if the domain contains three elements [d1, d2, d3] and the range contains four elements [r1, r2, r3, r4]? Do we ignore the extra range element r4 in that case? Or do we ignore the domain element d3 and partition [d1, d3] uniformly into three intervals [r1, r2], [r2, r3], and [r3, r4]? Do we partition [d1, d2] using [r1, r2, r3], and partition [d2, d3] using [r3, r4]? I think it’s better to either be explicit and require parallel arrays or for the user to provide an interpolator, say using d3.piecewise. We could provide shorthand syntax for d3.piecewise, but I don’t think we should do it just by allowing the range to contain extra elements; there should be a more explicit signal.

  • I added a warning when we detect extra elements in the domain or range.

@Fil
Copy link
Contributor

Fil commented May 25, 2023

Thanks for the detailed answer! I was just reading the documentation for scales to clarify my comment—and I realize I can't find any mention of polylinear or piecewise scales.

@mbostock mbostock merged commit 90d20a5 into main May 25, 2023
@mbostock mbostock deleted the mbostock/fix-misaligned-range branch May 25, 2023 16:42
This was referenced May 25, 2023
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* fix misaligned range

* warn on extra elements
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.

Continuous color ranges with more than two elements are handled inconsistently
2 participants