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

A generic filter transform to hide marks (perhaps usable together with sort for line and area)? #138

Closed
mbostock opened this issue Feb 24, 2021 · 7 comments · Fixed by #205
Labels
enhancement New feature or request

Comments

@mbostock
Copy link
Member

mbostock commented Feb 24, 2021

In other words, a more convenient way of specifying a mark transform when you want to filter the data.

@mbostock mbostock added the enhancement New feature or request label Feb 24, 2021
@Fil
Copy link
Contributor

Fil commented Feb 25, 2021

More convenient than transform: data => data.filter(test) would be to write filter: test?

@mbostock
Copy link
Member Author

Exactly, that’s all this is.

@Fil
Copy link
Contributor

Fil commented Feb 25, 2021

shouldn't this happen in the Mark constructor, right after this.data = arrayify(data);?

I'm struggling a bit with this, in particular because the current approach regarding transforms delays them for faceting, but I think filtering should not (if you facet on z and filter out a certain z, you don't want the empty facet to appear!).

For example in src/marks/area.js it seems that we "erase" a transform passed by the user if they also pass a sort:
Plot.areaY(data, { transform: blah, sort: bleh }) // blah is ignored

Now, if we want to specify, eg:
Plot.areaY(...Plot.stackY(data, { transform: t0, sort: s, filter: f }))
we want the transforms to be composed, not replaced: the resulting transform t1 would be (data, facets) => stacked(arrayify(data).filter(f), facets), which might decide to consume sort and (or?) push it up to the areaY mark.

(Plus, of course, filtering out a z value should not break the faceting system…)

@Fil Fil mentioned this issue Feb 25, 2021
@mbostock
Copy link
Member Author

The idea is you can specify sort or filter as a convenient shorthand for transform, but if you specify transform, it takes priority because it’s more explicit — but you really shouldn’t do that as the transform is the more general form and it’s meaningless to specify both. I don’t think we should try to compose sort & filter together with the more general transform.

@Fil
Copy link
Contributor

Fil commented Feb 28, 2021

A simple use case for transform composition is when you want to groupX (from #176) filtered data. I've given it a shot in https://observablehq.com/d/16561c3e9010bffc ; it still seems like a lot of code to achieve this very simple request.

The most "obvious" route is, of course, to write Plot.mark(data.filter(test), options), but it breaks faceting; maybe that's the original issue to revisit?

@mbostock
Copy link
Member Author

You can workaround the faceting issue by moving the filter up above the plot call:

data = data.filter(test);

I like the idea of extracting transform composition from the transforms themselves. Supporting transform composition isn’t trivial because of needing to handle facet-aware and facet-unaware transforms, and it’d be even more tedious if we also want transforms to support the filter (and sort, and maybe map) shorthand. And likewise making each transform implementation separately responsible for composition makes it harder for folks to write well-behaved transforms outside of Plot.

It feels a little weird to pass the filter option to Plot.groupX, and what if you want to chain multiple filters? So how about associating the filter function more directly with the filter transform?

Plot.barY(data, Plot.filter(d => d > 0, Plot.groupX({fill: d => d[0]})))

Though I guess now that’s a little inconsistent, too, since we don’t separate the group-specific and mark options when using Plot.groupX. 🤔 If we did, what would look like this?

Plot.barY(data, Plot.filter(d => d > 0, Plot.groupX(d => d, {fill: d => d[0]})))

And presumably Plot.group would take x, y, options as arguments? Though that doesn’t really work because these transforms can take additional options, which would mean two options objects, presumably. Maybe that’s not a terrible thing but I do think it slightly increases the notational viscosity to add/remove transforms.

Another thought is that this is moving us farther away from the transform option. But maybe we could change this option to allow transforming not just the data but also the channel definitions, somehow. E.g.,

Plot.plot({
  marks: [
    Plot.barY(data, {fill: "0", transform: Plot.groupX()})
  ]
})

And then, maybe we could also extend the transform option to be an array and allow multiple transformations to be chained in sequence? (Here a function is shorthand for a simple data transformation.)

Plot.plot({
  marks: [
    Plot.barY(data, {fill: "0", transform: [data => data.filter(d => d > 0), Plot.groupX()]})
  ]
})

Or with additional shorthand:

Plot.plot({
  marks: [
    Plot.barY(data, {fill: "0", transform: [Plot.filter(d => d > 0), Plot.groupX()]})
  ]
})

@Fil
Copy link
Contributor

Fil commented Feb 28, 2021

data = data.filter(test);

yes this works in the simple example, but not in a complex plot with several marks, one of which needs to be filtered (which is what we'll need to add labels, for example, as discussed in #174).

I like the idea of an array of transforms!

This was referenced Mar 1, 2021
Fil added a commit that referenced this issue Mar 5, 2021
@Fil Fil mentioned this issue Mar 5, 2021
Fil added a commit that referenced this issue Mar 9, 2021
closes #138
supersedes #194
@Fil Fil mentioned this issue Mar 9, 2021
mbostock pushed a commit that referenced this issue Mar 9, 2021
closes #138
supersedes #194
mbostock added a commit that referenced this issue Mar 9, 2021
* {filter, transform}

closes #138
supersedes #194

* generic sort, filter

* tolerate null

Co-authored-by: Philippe Rivière <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants