Skip to content

the default sorting of Plot.dot didn't work with Plot.hexbin#889

Closed
Fil wants to merge 2 commits into
mainfrom
fil/dot-sort-fixes
Closed

the default sorting of Plot.dot didn't work with Plot.hexbin#889
Fil wants to merge 2 commits into
mainfrom
fil/dot-sort-fixes

Conversation

@Fil
Copy link
Copy Markdown
Contributor

@Fil Fil commented May 30, 2022

… because options.r was undefined when we initialized the mark (i.e. before the initializer is called). In that case we now pass a channel (an empty array), allowing Plot.dot to set up the default sort. The channel will be overridden by the initializer.

Sorry I didn't catch this when reviewing #801

… because options.r was undefined when we initialized the mark (i.e. before the initializer is called). In that case we now pass a channel (an empty array), allowing Plot.dot to set up the default sort. The channel will be overridden by the initializer.
@Fil Fil requested a review from mbostock May 30, 2022 16:21
@mbostock mbostock mentioned this pull request May 30, 2022
Copy link
Copy Markdown
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.

This seems like a good idea, even if we also want to allow optional channels in index sorting as proposed by #891.

Copy link
Copy Markdown
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.

Actually, I’m not sure whether I prefer this or #891.

Since initializers run after mark construction, there’s no longer a guarantee that the mark will know which channels will be available on render at the time the mark constructor is called. So either we say it should be the responsibility of the initializer to indicate that the channels will be available (which this PR does only for the r channel on the hexbin transform, but not in general); or we say that marks shouldn’t assume to know which channels will be available. #891 takes the latter approach which feels a little bit more robust in this regard, even though the constructor is arguably “confused” about what will happen on render.

@Fil
Copy link
Copy Markdown
Contributor Author

Fil commented May 30, 2022

I think I prefer #891 because this felt a bit like a hack. However, if we wanted to have the transform "inform" the mark that it might create a new channel, we could introduce a specific symbol "delayed channel" rather than this []. There might be more use cases later.

@mbostock
Copy link
Copy Markdown
Member

Sounds good. Closing in favor of #891 then.

@mbostock mbostock closed this May 31, 2022
@Fil Fil deleted the fil/dot-sort-fixes branch May 31, 2022 06:14
@mbostock mbostock mentioned this pull request Jun 9, 2022
10 tasks
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