Skip to content

Conversation

@Fil
Copy link
Contributor

@Fil Fil commented Jan 27, 2022

implements my comment #648 (comment)

The layout is called once, with an index that is an array of arrays, one for each facet.

this addresses two concerns:

  1. makes it possible to create "smarter" layouts that can work on either the elements of each facet or the whole set of elements
  2. performance (memory allocation is done once for all the facets)

the cost is a slightly more complex function to write, because it needs to iterate on the arrays in index; but this is consistent with what we do in transforms.

test the dodge layout with a channel r
@Fil Fil requested a review from mbostock January 27, 2022 14:48
Fil added a commit that referenced this pull request Jan 27, 2022
note: the custom layout example will change a little if we do #712
This was referenced Jan 27, 2022
@mbostock
Copy link
Member

mbostock commented Jan 27, 2022

As you point out, there is a cost to doing this: an increase in the complexity of every layout. Therefore we should be confident that the added expressiveness (or performance) is worth the added complexity. In the case of transforms, we knew that transforms had to be facet-aware e.g. so that the same set of bins would be used across facets. It is not clear to me that the same is true of layouts: none of the current or proposed layouts appear to require knowledge of facets. Also it is not clear that the performance cost of the current approach is appreciable.

Even if we want facet-aware layouts in the future, it seems likely that non-facet-aware layouts will be the common case. I expect we could design a mechanism to opt-in to (more complex) facet-aware layouts, while being backwards-compatible for non-facet-aware layouts, letting most layouts remain simple

@Fil Fil closed this Jan 27, 2022
@Fil Fil deleted the fil/layout-all-facets branch April 5, 2023 14:59
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.

3 participants