Skip to content
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

Fil/group reduce group domain #275

Closed
wants to merge 52 commits into from
Closed

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 27, 2021

merges #271 into #272

@Fil Fil requested a review from mbostock March 27, 2021 11:17
Comment on lines 56 to 58
// Only return groups that belong to the domain
const xdefined = GX && maybeDomain(xdomain);
const ydefined = GY && maybeDomain(ydomain);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve been thinking about this a little more, and I think it makes more sense that the corresponding scale’s domain drive which groups are shown, rather than needing to pass the domain explicitly to the group transform. I think this means that we should remove any filtering here, and just group all the data, and then we simply won’t show the resulting marks (e.g., bar will filter it out using maybeCoords).

This now intersects with the “grand unification” of group and reduce I’ve been working on, so I’ll try to merge these two PRs.

@mbostock's comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We’ll need to think through how to apply this more generally. This problem isn’t specific to bars — it applies whenever an ordinal scale is in play, which could be with any mark.

comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the domain option.

Remains to check if this works consistently across all marks.

@mbostock
Copy link
Member

mbostock commented Mar 27, 2021

I’ve been thinking that we should map the channel through the scale, e.g., XX = X.map(x), then pass that to filter I, and use XX to set the attributes.

@mbostock
Copy link
Member

I guess that’s basically what you’re doing, but sparsely. I wonder if there’s a way to do it for the entire channel once. Maybe I’m over-optimizing. Or maybe it could be done automatically by Plot since Plot already knows which channels are bound to which scales.

@mbostock
Copy link
Member

(Re. the precomputed scaled channels idea, I mentioned in #149 that this might allow other interesting things such as varying scales per facet or parallel coordinates.)

Base automatically changed from mbostock/group-reduce to main March 28, 2021 01:04
@Fil
Copy link
Contributor Author

Fil commented Mar 29, 2021

superseded by #280

@Fil Fil closed this Mar 29, 2021
@Fil Fil deleted the fil/group-reduce-group-domain branch March 29, 2021 07:20
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.

None yet

2 participants