Skip to content

filter, sort, and reverse transforms #472

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

Merged
merged 6 commits into from
Aug 10, 2021
Merged

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Aug 1, 2021

This exports Plot.filter(value, options), Plot.sort(value, options) and Plot.reverse(options) so that the order in which these transforms are applied can be specified explicitly when the basic order of filter → sort → reverse is insufficient. Also, it makes possible to apply filtering, sorting, and reverse before or after another transform, such as binning.

Ref. #439 (comment)

@mbostock mbostock requested a review from Fil August 1, 2021 14:58
@Fil
Copy link
Contributor

Fil commented Aug 3, 2021

I wanted to add documentation and examples, but I found myself struggling with the seemingly simple task of sorting bins by y (large to small) before stacking them.

In https://observablehq.com/d/44c5fa3c52156203:

This works:

Plot.barY(
  data,
  Plot.reverse(
    Plot.sort(
      "length",
      Plot.groupX({ y: "count" }, { x: "island", fill: "sex" })
    )
  )
).plot()

But this doesn't:

Plot.barY(
  data,
  Plot.reverse(
    Plot.sort(
      "y",
      Plot.groupX({ y: "count" }, { x: "island", fill: "sex" })
    )
  )
).plot()

that's because the Plot.sort accessor receives the bins as input, and not the derived y channel.

(both have an implicit stacking transform added)

@mbostock mbostock force-pushed the mbostock/sort-filter-reverse branch from 2060947 to e234e37 Compare August 3, 2021 14:34
@mbostock
Copy link
Member Author

mbostock commented Aug 3, 2021

Plot doesn’t have a way to refer to another channel definition, like an alias. For example, you can’t do something like:

Plot.barY(data, {x: "revenue", y: "name", fill: "category", stroke: "fill"})

The first argument to Plot.sort is akin to a value in the options object, like sort: "field", and so you can’t replace "field" with the name of another channel such as "y"; you can only give it the name of a column (or a value accessor or a comparator). So, needing "length" here instead of "y" makes sense to me.

If we want to have a way to alias channels, we’d need a way to disambiguate it from data fields, and to make sure there are no circular definitions… and then there’s the question of whether you could refer to other channels in a function accessor, or if it can only be used to alias one channel onto another channel. In any case that feels like out of scope for this PR and possibly a significant undertaking.

@Fil
Copy link
Contributor

Fil commented Aug 4, 2021

Makes sense! I hope the tentative documentation I added is clear enough about this.

@mbostock mbostock mentioned this pull request Aug 4, 2021
@mbostock
Copy link
Member Author

mbostock commented Aug 4, 2021

I am not sure how to address the inconsistency with #480. Do you have a preference, @Fil? If we take that approach, it would mean something like this:

Plot.sort({sort: "y", ...Plot.groupX({y: "count"}, {x: "island", fill: "sex"})})

which is, well, obviously awkward.

I suppose I could go back to Plot.normalizeX(basis, options) etc. in the other PR, but then I feel like we’d want Plot.windowX(windowOptions, markOptions) or something, which is also somewhat awkward because the markOptions aren’t strictly only for the mark—they include channels that will be transformed by the transform. Maybe that’s still the best option?

@mbostock
Copy link
Member Author

mbostock commented Aug 4, 2021

Here is the shorthand for the normalize transform:

https://github.com/observablehq/plot/compare/mbostock/normalize-shorthand

I have a guess as to how to extend this to the window transform, but it seems a little weird. I’m unclear on whether I should further try to extend the pattern to the stack transform, but if we did, it feels related to the other discussion about the reverse option being consumed by transforms #439 rather than representing the basis reverse transform (which in the case of stack would be applied prior to stacking).

@Fil
Copy link
Contributor

Fil commented Aug 4, 2021

If I understand correctly the problem with window is that we have several options, {k:24, anchor: "start"}, whereas for sort we have only 1, and write it directly as "length" instead of {sort: "length"}?

A common approach might be to merge the two arguments if they are both objects?
https://observablehq.com/d/a05b4fd47e766b26

@mbostock
Copy link
Member Author

mbostock commented Aug 4, 2021

To recap, there are really two problems here.

The first is that we want a way to control the order of the basic transforms (filter, sort, and reverse). Currently we cannot control the order because they are applied by maybeTransform, so the order is always filter → sort → reverse followed by any other transforms (which you can control the order of as transform2(transform1(…)) etc.).

The second is that sometimes the options to transforms are ambiguous: sometimes when we chain transforms, we want those options to apply to specific transforms in the chain. However that is not possible with the current form since the options for everything is declared as a single input into the chain: transformN(…transform2(transform1(options))).

We fixed the first issue by promoting the basic transforms to explicit transforms, so that you can chain them like the other transforms. However, we still need a fix for the second issue. And I’m considering pulling out the transform-specific options to a separate argument, as in transform(transformOptions, otherOptions). But I’m not sure that’s necessary in all cases or just in some cases, etc.

@mbostock mbostock force-pushed the mbostock/sort-filter-reverse branch from a45b334 to 99da733 Compare August 10, 2021 22:28
@mbostock
Copy link
Member Author

FYI I removed the example of composing basic transforms because I worry people would use it over the stack transforms order and reverse options, which are more likely to be correct. It’d be nice to have an example there but I think the application is fairly niche so it’s not urgent.

@mbostock mbostock merged commit 21a0caa into main Aug 10, 2021
@mbostock mbostock deleted the mbostock/sort-filter-reverse branch August 10, 2021 22:37
@reubano reubano mentioned this pull request May 4, 2022
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.

2 participants