Skip to content

the bin transform should not consume {reverse} #439

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

Closed
wants to merge 1 commit into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 20, 2021

since it doesn't make use of it… instead pass it up so we can stack the resulting bins in the desired order.
solves https://stackoverflow.com/questions/68056843/in-observable-plot-how-to-sort-order-the-stack-from-a-bin-transform/68057660

build and test: https://observablehq.com/@fil/stack-bins-ordered-439

@Fil Fil requested a review from mbostock June 20, 2021 16:29
@Fil Fil changed the title the bin transform should not consume {reverse} since it doesn't make … the bin transform should not consume {reverse} Jun 20, 2021
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

My first thought was “shouldn’t the bin transform pass through all options that it doesn’t understand?” My second thought was “doesn’t it do that already?” And yes, it does; but the reverse option is consumed by maybeTransform, so the bin transform does in fact make use of this option:

if (r1) t1 = compose(t1, reverse);

So, isn’t the problem here that we in effect want the reverse transform to apply after the bins have been created, rather than before (where it has almost no effect)? I guess it’s okay to change the behavior for the bin transform specially, but it seems a little inconsistent with how the basic transforms work. What do you think?

@Fil
Copy link
Contributor Author

Fil commented Jun 20, 2021

Same analysis — I didn't see a better way to fix this than by having the bin transform explicitly "save it for later", since it makes no sense to order the elements in the bins. But I agree it goes a little against the genericity of the transforms composition.

@mbostock
Copy link
Member

It raises the question: should the sort transform then similarly apply after the bins have been constructed, rather than before?

@Fil
Copy link
Contributor Author

Fil commented Jun 21, 2021

There's one case where sort must be done before stacking: when we compute the wiggle offset (which minimizes the total movement), the data should be sorted according to the stack's "location" dimension (x if we're using stackY).
#177 (comment)
However in that use case: 1. it doesn't matter if it's done before or after binning 2. it doesn't matter if the data are sorted or sorted reverse—they just have to be "aligned".

If we need to be explicit about where {reverse} is applied (or want to apply it several times with different targets), there's always the possibility of using a combination of spread (as in this example) and Plot.transform (#411).

The full example would become something like:

Plot.rectY(
  data,
  Plot.stackY({
    ...Plot.binX(
      { y: "count", fill: "first", title: (d) => JSON.stringify(d[0]) },
      Plot.transform({
        x: "body_mass",
        fill: "species",
        z: "species",
        // order: "value" // ***
        sort: "sex",
        reverse: true // applies on "sort", consumed by Plot.transform hence not passed up by Plot.bin
      })
    ),
    order: "value", // alternatively, could be defined in ***
    reverse: true // applies on "order"
  })
).plot()

Also related #297

@Fil Fil force-pushed the fil/fix-bin-reverse branch from 0efda2c to e00c583 Compare July 21, 2021 07:25
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

The main point I’m trying to make here is that sort and reverse should always travel together: you need both to do descending sort (at least, for convenience). Otherwise I’m okay making an exception for the bin transform to not consume these options, and instead pass them through (as here, by removing them from options so they don’t get consumed by maybeTransform).

Should we make the same exception for the group transform?

@mbostock mbostock force-pushed the fil/fix-bin-reverse branch from e00c583 to 6e3b1bd Compare August 1, 2021 14:09
@mbostock
Copy link
Member

mbostock commented Aug 1, 2021

This is what I meant

fil/fix-bin-reverse...mbostock/fix-bin-sort

but thinking about it some more, I’m not sure it makes sense: if the bin transform is composed with the stack transform, then yes, the reverse option will be consumed by the stack transform. But the sort option will not be consumed at all—it’ll just be ignored—since that sort option is only consumed by maybeTransform (which we’re talking about bypassing).

Maybe the root of the confusion here is that there are two different reverse options, one for the stack transform, and one for the basic transform:

function stack(x, y = () => 1, ky, {offset, order, reverse, ...options} = {}) {

reverse: r1,

Here we want to pass through the reverse option to the stack transform, but not the reverse option to the basic transform (which should travel together with the sort option, unlike the stack transform, which uses an order option).

Perhaps being explicit about who the reverse option is intended for is desirable here. This option is meaningful for the bin transform (together with the sort option), albeit in a subtle way affecting the order of the data within the bin. That suggests saying stackY({...binX(…), reverse: …}) if you do want to pass the reverse option explicitly to the stack transform.

But we still don’t have a way to apply sort and reverse after binning, because the basic transform always composes transforms in a fixed order:

plot/src/mark.js

Lines 235 to 237 in 7e45462

if (f1 != null) t1 = filter(f1);
if (s1 != null) t1 = compose(t1, sort(s1));
if (r1) t1 = compose(t1, reverse);

This makes me think that maybe the basic transforms should be pulled out into functions, so you can say binX(sort(order, options)) if you want to sort before binning and sort(order, binX(options)) if you want to sort after binning. And similarly filter(test, options) to replace the filter option and reverse(options) to replace the reverse option. We should ideally maintain backwards-compatibility even if we deprecate the existing options, though.

@mbostock
Copy link
Member

Closing in favor of #494.

@mbostock mbostock closed this Aug 10, 2021
@reubano reubano mentioned this pull request May 4, 2022
@Fil Fil deleted the fil/fix-bin-reverse branch January 24, 2023 11:45
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