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

exploring initializers composition #818

Closed
wants to merge 13 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Mar 22, 2022

  1. Plot.selectFirst should be able to select the first hexbin
  2. Plot.hexbin should be able to operate on scaled X and Y channels

Note: I've also tested merging this branch with fil/reinitialize-dodge and composing Plot.hexbin (Plot.dodgeX(…)).

@Fil Fil mentioned this pull request Mar 22, 2022
16 tasks
@mbostock
Copy link
Member

mbostock commented Mar 22, 2022

Yeah, we definitely need to avoid a duplicate implementation (once as a “data transform” and the other as a “channel transform” a.k.a. “initializer”). I think the options here are either (1) data transforms always run before channel transforms, and hence you can’t run the select transform after the hexbin transform, or (2) we somehow generalize it so that data transforms and channel transforms are the same thing, and you can somehow have an arbitrary dependency graph for channels that depend on other channels.

I expect that (2) would take us at least another month to figure out, so I am inclined to not attempt this at all and just treat data transforms as a different class of transform than channel transforms.

Another thought is that we could throw an error (or issue a warning) if you apply them in the wrong order. E.g., the basic transform helper could check if the initializer option is set, and throw an error, thereby requiring you to set the initializer last. That would at least make it more obvious that you can’t Plot.select(Plot.hexbin(…)) but you can Plot.hexbin(Plot.select(…)).

@Fil
Copy link
Contributor Author

Fil commented Mar 23, 2022

i've reverted the changes that made Plot.select work both as a data or a channel transform, and we now throw an error if a data transform is applied after a channel transform.

@Fil Fil marked this pull request as ready for review March 23, 2022 14:07
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.

Do we need to implement initializer composition within the basic transform helper? My impression is that could be implemented in a separate helper like we did before. The basic transform helper could still check that the incoming initialize option is nullish and error if any transform is specified.

@Fil
Copy link
Contributor Author

Fil commented Mar 23, 2022

Possibly, but the difficulty is that a channel transform can make use of both functions (in hexbin, transform is used to initialize the output reducers, and initialize to create the bins). So at least to check if it's ok to compose, it can't be done as two independent tests. 🤔

@mbostock
Copy link
Member

mbostock commented Mar 23, 2022

I don’t see the problem. The hexbin transform will declare both a transform (data transform) and an initializer (channel transform); the former will error if there’s already an initializer declared, which is what we want. (We don’t expect any initializers to run before the hexbin transform?) If someone tries to declare another transform (data transform) after the hexbin transform, that will also error because the hexbin transform declared an initializer.

@Fil
Copy link
Contributor Author

Fil commented Mar 23, 2022

A concrete use case would be Plot.hexbin + Plot.dodgeY (which may or may not make sense from a dataviz standpoint).

Capture d’écran 2022-03-23 à 17 50 38

Maybe the problem here is that Plot.hexbin uses a data transform to initialize its reducers—it could probably do this in the initializer part of the code instead.

I'm also starting to get lost between all the branches I have opened :)

@mbostock
Copy link
Member

Maybe the problem here is that Plot.hexbin uses a data transform to initialize its reducers—it could probably do this in the initializer part of the code instead.

Yeah, the only reason the hexbin transform does this is that it needs access to the (possibly transformed) data, which is not normally passed to an initializer. If we passed state.data to mark.reinitialize, then we wouldn’t need a trivial data transform to access the data. Let me take a crack at that.

const {facets, channels} = mark.reinitialize(state.facets, state.channels, scales);

@mbostock
Copy link
Member

Okay, done in 42ac4f0. Seem good?

@Fil
Copy link
Contributor Author

Fil commented Mar 23, 2022

yes!

Comment on lines +29 to +30
const x = X.scale !== undefined ? scales[X.scale] : identity.transform;
const y = Y.scale !== undefined ? scales[Y.scale] : identity.transform;
Copy link
Member

Choose a reason for hiding this comment

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

This is fixing the behavior of the hexbin transform when identity scales are used?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, no, since we still have scale objects when position scales use the identity scale type…

So this is instead respecting the incoming channel’s scale definition, rather than assuming that the x channel is bound to the x scale and the y channel is bound to the y scale. That’s interesting and I hadn’t considered that possibility; all the built-in marks bind x and y to the corresponding scales, but it is theoretically possible for a custom mark not too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's for when you compose layouts, and the previous layout has returned channels that are already scaled. Typically hexbin + dodgeY leads to this situation.

Copy link
Member

Choose a reason for hiding this comment

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

Aha. Thanks for the context.

@Fil Fil mentioned this pull request Mar 24, 2022
@mbostock mbostock force-pushed the mbostock/reinitialize branch 2 times, most recently from b305131 to 886615b Compare May 4, 2022 22:26
@mbostock
Copy link
Member

mbostock commented May 7, 2022

Merged into #823, which was merged into #801.

@mbostock mbostock closed this May 7, 2022
@Fil Fil deleted the fil/initialize-composition branch April 5, 2023 14:58
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