Skip to content

"sort fy by x" bug #840

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

Closed
wants to merge 1 commit into from
Closed

"sort fy by x" bug #840

wants to merge 1 commit into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Apr 5, 2022

This branch is trying to fix the following issue:

When we have "sort fy by x" and a transform, the series are not indexed properly. The channel to sort (XV, in this case fy) is indexed by the original data, and the channel by which to sort (YV, in this case x) is indexed by the transformed (grouped) data.

It can be solved, in the case of this unit test, by grouping data by series:

const XV = x === "fy" || x === "fx" ? [...new Set(X[1].value)] : X[1].value;

However this is not a proper fix, since it should apply only when data is indeed grouped this way. We have to understand how the data is reindexed, or alternatively we have to make the facet information follow along with the grouping.

Note: the broken test is the expected chart, but we can't achieve it yet without breaking a few others:

Capture d’écran 2022-04-05 à 23 24 20

cc: @enjalot

When we have "sort fy by x" and a transform, the series are not indexed properly.
The facet channel (XV) is indexed by the original data, and the x channel (YV) is indexed by the transformed (grouped) data.

It can be solved, in the case of this unit test, by grouping data by series:
> const XV = x === "fy" || x === "fx" ? [...new Set(X[1].value)] : X[1].value;

however this is not a proper fix, since it should apply only when data is indeed grouped this way.
@Fil Fil added the bug Something isn’t working label Apr 5, 2022
@Fil
Copy link
Contributor Author

Fil commented Apr 6, 2022

Forgot to mention that a solution is to avoid doing the transform, and use e.g.:

    Plot.dotX(agediff, {
      x: "age_difference",
      stroke: null, // don't display any mark
      sort: {fy: "x", reduce: "median"}
    }),

(so maybe the bug can be solved by warning that there is a mismatch?)

@mbostock
Copy link
Member

mbostock commented Dec 2, 2022

I’m having difficulty following this issue. 🤯

@mbostock
Copy link
Member

mbostock commented Mar 4, 2023

I think this is only a bug with the facet channels, right? Normal channels should be transformed consistently; it’s just the facet channels that don’t respect the transforms. I made the same mistake at first in #1304 and fixed it in 41aad80. There’s probably some similar special logic we need here to handle the facet channels when there is a transform (reconstructing them from index.fx and index.fy).

@Fil Fil deleted the fil/fix-sort-by-transform branch April 5, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants