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.

Sign up for GitHub

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

fix centroid/geocentroid with tips #2086

Closed
wants to merge 4 commits into from
Closed

fix centroid/geocentroid with tips #2086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bef7e75
closes #2085
Fil Jun 14, 2024
e065a5f
provenance information
Fil Jun 14, 2024
b40486f
consider the source key of a {transform} value
Fil Jun 14, 2024
c852ca1
better london map
Fil Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/channel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@ import {registry} from "./scales/index.js";
import {isSymbol, maybeSymbol} from "./symbol.js";
import {maybeReduce} from "./transforms/group.js";

export function createChannel(data, {scale, type, value, filter, hint, label = labelof(value)}, name) {
if (hint === undefined && typeof value?.transform === "function") hint = value.hint;
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;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? Should we do this in the column helper in options.js instead (per other comment)?

Copy link
Contributor Author

@Fil Fil Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is to prevent the tip in londonCarAccessGeoCentroid from displaying x and y, which are meaningless pixel values. I'll try and use column.

}
return inferChannelScale(name, {
scale,
type,
value: valueof(data, value),
label,
filter,
hint
hint,
source
});
}

Expand Down
2 changes: 2 additions & 0 deletions src/marks/geo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export class Geo extends Mark {
data,
{
geometry: {value: options.geometry, scale: "projection"},
x: {value: options.x, scale: "x", optional: true},
y: {value: options.y, scale: "y", optional: true},
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? It doesn’t look like these channels are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

r: {value: vr, scale: "r", filter: positive, optional: true}
},
withDefaultSort(options),
Expand Down
13 changes: 9 additions & 4 deletions src/transforms/centroid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,22 @@ export function centroid({geometry = identity, ...options} = {}) {
facets,
channels: {
x: {value: X, scale: projection == null ? "x" : null, source: null},
y: {value: Y, scale: projection == null ? "y" : null, source: null}
y: {value: Y, scale: projection == null ? "y" : null, source: null},
geometry: {value: G}
}
};
});
}

export function geoCentroid({geometry = identity, ...options} = {}) {
export function geoCentroid(options = {}) {
let C;
const {geometry = identity} = options;
return {
...options,
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),
Copy link
Member

Choose a reason for hiding this comment

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

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.

source: null
},
y: {transform: () => Float64Array.from(C, ([, y]) => y), source: null}
};
}
8 changes: 8 additions & 0 deletions test/data/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ CBO
https://www.cbo.gov/topics/budget/accuracy-projections
https://observablehq.com/@tophtucker/examples-of-bitemporal-charts

## london.json
giCentre, City University of London
https://github.com/gicentre/data

## london-car-access.csv
Derived by Jo Wood from UK Census data
https://github.com/observablehq/plot/pull/2086

## metros.csv
The New York Times
https://www.nytimes.com/2019/12/02/upshot/wealth-poverty-divide-american-cities.html
Expand Down
34 changes: 34 additions & 0 deletions test/data/london-car-access.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
borough,y2001,y2011,y2021
City of London,0.380,0.306,0.228
Barking and Dagenham,0.621,0.604,0.652
Barnet,0.733,0.713,0.701
Bexley,0.763,0.763,0.776
Brent,0.627,0.570,0.559
Bromley,0.770,0.765,0.771
Camden,0.444,0.389,0.364
Croydon,0.702,0.665,0.664
Ealing,0.683,0.647,0.632
Enfield,0.715,0.675,0.690
Greenwich,0.592,0.580,0.569
Hammersmith and Fulham,0.514,0.448,0.425
Haringey,0.535,0.482,0.473
Harrow,0.773,0.765,0.753
Havering,0.767,0.770,0.785
Hillingdon,0.783,0.773,0.777
Hounslow,0.714,0.684,0.672
Islington,0.424,0.353,0.331
Kensington and Chelsea,0.496,0.440,0.417
Kingston upon Thames,0.762,0.749,0.743
Lambeth,0.491,0.422,0.420
Lewisham,0.572,0.519,0.523
Merton,0.699,0.674,0.670
Newham,0.511,0.479,0.483
Redbridge,0.738,0.721,0.725
Richmond upon Thames,0.763,0.753,0.746
Southwark,0.481,0.416,0.397
Sutton,0.767,0.766,0.772
Tower Hamlets,0.432,0.370,0.336
Waltham Forest,0.610,0.581,0.579
Wandsworth,0.593,0.547,0.521
Westminster,0.436,0.371,0.338
Hackney,0.440,0.354,0.351
Loading