Skip to content

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.

3 participants