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

promote geometry to x and y for tip #2088

Merged
merged 5 commits into from
Jun 16, 2024
Merged

promote geometry to x and y for tip #2088

merged 5 commits into from
Jun 16, 2024

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Jun 14, 2024

Fixes #1743. Fixes #2085. Fixes #2087. This makes the tip option “just work” with the geo mark. In the future, it’d be nice to have more control — say to use the geoCentroid transform instead of the centroid transform, or to compute the closest polygon instead of the closest centroid. But maybe this is okay for now?

@mbostock mbostock marked this pull request as ready for review June 14, 2024 21:30
@Fil
Copy link
Contributor

Fil commented Jun 15, 2024

This is much easier to use and helps with #1743 (and maybe closes it? a secondary issue is the default tip contents, which I imagined could be based on the feature’s properties, if the geometry is a feature).

I still find it a bit troubling (in principle) that these two codes behave differently, with Plot.centroid “eating” the geometries if it is passed a non-default geometry option:

Plot.geo(access.map((d) => boroughs.get(d.borough)), Plot.centroid()) // displays the geometries
Plot.geo(access, Plot.centroid({geometry: (d) => boroughs.get(d.borough)})) // nothing is displayed

This can lead to surprising combinations: the code below draws the original shapes, and displays a tooltip on the geoCentroid:

Plot.geo(
  access.map((d) => boroughs.get(d.borough)),
  Plot.centroid({
    title: d => d.id,
    tip: true,
    geometry: (d) => ({type: "Point", coordinates: d3.geoCentroid(d)})
  })
)

But if you remove Plot.centroid, then the Point geometries are drawn instead of the shapes:

Plot.geo(
  access.map((d) => boroughs.get(d.borough)),
  {geometry: (d) => ({type: "Point", coordinates: d3.geoCentroid(d)}), tip: true}
)

Lastly, I also want to explore a better default placement based on the pole of inaccessibility (#1587) — but this doesn't have to be blocking and can be done as a follow-up.

@mbostock
Copy link
Member Author

From my perspective, it never makes sense to use the centroid or geoCentroid transform with the geo mark. The centroid and geoCentroid transform turn {geometry} into {x, y}. The geo mark accepts {geometry} while most other marks accept {x, y}. In other words, the centroid transforms turn a square peg into a round peg, while the geo mark is a square hole.

Having the centroid and geoCentroid transforms also pass through the geometry channel could work (instead of consuming them), but then we get back to the problem of the geo mark or the geoCentroid transform being responsible for declaring x and y channels even if it doesn’t need them directly. But it could do that only if the tip option were truthy, under the principle that the tip option is something all marks should support.

I can look into this approach tonight… it has the nice property that if you use the geoCentroid transform explicitly, it would disable the default centroid transform used by the tip option.

@mbostock
Copy link
Member Author

mbostock commented Jun 15, 2024

Okay, some big changes in the latest commit. You can now use the centroid or geoCentroid transform, or even to specify the x and y channels explicitly, in conjunction with the geo mark’s tip option. If you don’t specify x or y explicitly, then the geo mark will implicitly apply the centroid transform when the tip option is specified. I think this gives the most flexibility and the most convenience, and it doesn’t require any special treatment in derive.

In addition, the centroid and geoCentroid transforms now propagate the (memoized) geometry channel instead of consuming it, just in case the downstream mark still wants access to it. I also (necessarily) made the geoCentroid transform more robust; it shouldn’t assume that the x channel is computed before the y channel (and before the geometry channel); in some cases only a subset of channels will be computed.

Lastly I optimized memoize1 for the single-argument case, and fixed the equality test which should use Object.is.

@Fil Fil mentioned this pull request Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants