fix centroid/geocentroid with tips #2086
Conversation
|
I derived the car access data from the UK Census (all covered by Open Government Licence v3.0. Specifically the local authority level spatial aggregations for:
(proportions calculated by me by comparing with total households available in the same tables). Spatial London borough data derived from https://geoportal.statistics.gov.uk/datasets/caaf81b26a45453fa4849dbce31a3293_0/explore by selecting, simplifying and reprojecting in MapShaper. |
|
This is ready to review. |
There was a problem hiding this comment.
I would love more comments to accompany code changes, either in the PR description, in self-review comments, or in the code itself. Without them I have to guess why the changes are necessary; this is a good exercise in active thinking, but it would be more efficient if you gave me some hints. Thank you!
| x: {transform: (data) => Float64Array.from((C = valueof(valueof(data, geometry), GeoCentroid)), ([x]) => x)}, | ||
| y: {transform: () => Float64Array.from(C, ([, y]) => y)} | ||
| x: { | ||
| transform: (data) => Float64Array.from((C = valueof(valueof(data, geometry), GeoCentroid)), ([x]) => x), |
There was a problem hiding this comment.
Since we’re no longer consuming the geometry option and instead making it available downstream, we need to pass through the materialized values so that they don’t get recomputed (especially important if the geometry option is specified nondeterministically e.g. with random values). You can use column from options.js for this.
| x: {value: options.x, scale: "x", optional: true}, | ||
| y: {value: options.y, scale: "y", optional: true}, |
There was a problem hiding this comment.
Why is this needed? It doesn’t look like these channels are used.
There was a problem hiding this comment.
They are used by the tip mark, which derives the x and y channels from the geo mark. That's visible when interacting with the londonCarAccessGeoCentroid example (but unfortunately not caught by the automated test).
There was a problem hiding this comment.
If I’m understanding correctly, you’re suggesting that the geo mark should be responsible for preparing the x and y channels for the tip option (and its derived tip mark) even though the geo mark doesn’t need these channels for itself.
This doesn’t feel like the right place to fix this problem. Either all marks should implicitly support x and y channels if declared under the expectation that maybe you’ll use the tip option and the derived tip mark will need them, or the geoCentroid transform should be responsible for declaring x and y as custom channels rather than simply passing them through as options.
Another possibility is that the geo mark should just support the tip option directly by implicitly invoking the geoCentroid transform (somehow?), or that the derive function for deriving a tip from an existing mark should similarly promote the geometry channel into x and y channels, or that the tip mark and pointer transform should directly support reading the geometry channel as an alternative to x and y channels.
| export function createChannel(data, {scale, type, value, filter, hint, source, label = labelof(value)}, name) { | ||
| if (typeof value?.transform === "function") { | ||
| if (hint === undefined) hint = value.hint; | ||
| if (source === undefined) source = value.source; |
There was a problem hiding this comment.
Why is this necessary? Should we do this in the column helper in options.js instead (per other comment)?
There was a problem hiding this comment.
This is to prevent the tip in londonCarAccessGeoCentroid from displaying x and y, which are meaningless pixel values. I'll try and use column.
|
A related question is should the tip option work directly with the geo mark (without requiring the geoCentroid transform explicitly)? It sure seems like that would be convenient. But using centroids for pointing at polygons maybe isn’t great, so I wonder if that’s an acceptable implementation, or if we should be thinking about other pointing mechanisms (e.g., SDF). |
|
(tracking the secondary issue of default tips in #2087) |
| Plot.geo( | ||
| access, | ||
| Plot.centroid({ | ||
| fx: "year", | ||
| geometry: (d) => boroughs.get(d.borough), | ||
| fill: "access", | ||
| stroke: "var(--plot-background)", | ||
| strokeWidth: 0.75, | ||
| tip: true | ||
| }) | ||
| ) |
There was a problem hiding this comment.
I guess I’m not sure we should/need to support this use case now. A more explicit way to write this is:
| Plot.geo( | |
| access, | |
| Plot.centroid({ | |
| fx: "year", | |
| geometry: (d) => boroughs.get(d.borough), | |
| fill: "access", | |
| stroke: "var(--plot-background)", | |
| strokeWidth: 0.75, | |
| tip: true | |
| }) | |
| ) | |
| Plot.geo(access, { | |
| fx: "year", | |
| geometry: (d) => boroughs.get(d.borough), | |
| fill: "access", | |
| stroke: "var(--plot-background)", | |
| strokeWidth: 0.75 | |
| }), | |
| Plot.tip( | |
| access, | |
| Plot.pointer(Plot.centroid({fx: "year", geometry: (d) => boroughs.get(d.borough), fill: "access"})) | |
| ) |
I don’t think it’s right to use the centroid/geoCentroid transform with the geo mark if the geo mark doesn’t need the x and y channels — if they’re just intended to be used with the tip option. It feels like we should just make the tip option work directly with the geo mark rather than going halfway.
There was a problem hiding this comment.
As I’ve said before, we should try to name tests more about the features they are testing than the datasets they are using. E.g., this should be a test of geo tips.
|
I’ve made a sketch at my alternate take in #2088. I think it could be further extended (for example maybe there’s an option to use the geoCentroid transform instead of the centroid transform, or maybe there’s an option to tell the pointer transform to use the nearest polygon instead of the nearest centroid — though in that case, we’d still need to pick an xy point to place the tip) but maybe it’s an acceptable interim solution. |
|
superseded by #2088 |
closes #2085
cc: @jwoLondon
(and can you give us provenance information for the dataset?)An imperfection is that x and y show up in the tip in the londonCarAccessGeoCentroid example.