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

grouped reduce in area and line marks #732

Closed
wants to merge 3 commits into from
Closed

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Feb 1, 2022

closes #724 (and #503)

todo:

  • avoid the multiplication of the reduced value(s) to the whole channel
  • reuse the group reducers if possible?

supersedes #508

@Fil Fil requested a review from mbostock February 1, 2022 16:54
@mbostock
Copy link
Member

mbostock commented Feb 1, 2022

This is still materializing a mapped channel even though we only need a single value. And as you noted it doesn’t use the existing reducers. I’ll try to take a crack at this soon, but I want something that does both of these things before merging.

@Fil
Copy link
Contributor Author

Fil commented Feb 1, 2022

yes a bit more work is needed. I wanted to rebase and clean up the previous PR so we could make progress on this.

To sum up the current status: this is suboptimal because it populates a channel with n copies of the reduced value. But I've been struggling with the fact that we can't reduce to a single value either: we need a channel, because we have to carry an element for each line or area. The difficulty is that the reduction must to happen in a grouped transform (z + facet, like map does), in a manner that is compatible with Plot.window, before we apply scales, and before we group.

Addendum: and the way the data is grouped is not necessarily happening in the same order in Plot.map and in Plot.line's render function.

@mcollie1
Copy link

mcollie1 commented Feb 7, 2022

👍 looking forward to this update

@mbostock
Copy link
Member

Should we close this, since we landed #761?

@Fil
Copy link
Contributor Author

Fil commented Mar 11, 2022

Yes. I still think there is value in the Plot.reduce helper described in #724 (comment).

@Fil Fil closed this Mar 11, 2022
@Fil Fil deleted the fil/reduce-grouped-styles branch March 11, 2022 10:57
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.

Reduce for line/area values?
3 participants