Skip to content

fix facet channel sort with transform #1316

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

Merged
merged 3 commits into from
Mar 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 17 additions & 2 deletions src/channel.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ export function inferChannelScale(name, channel) {
// Note: mutates channel.domain! This is set to a function so that it is lazily
// computed; i.e., if the scale’s domain is set explicitly, that takes priority
// over the sort option, and we don’t need to do additional work.
export function channelDomain(channels, facetChannels, data, options) {
export function channelDomain(data, facets, channels, facetChannels, options) {
const {reverse: defaultReverse, reduce: defaultReduce = true, limit: defaultLimit} = options;
for (const x in options) {
if (!registry.has(x)) continue; // ignore unknown scale keys (including generic options)
let {value: y, reverse = defaultReverse, reduce = defaultReduce, limit = defaultLimit} = maybeValue(options[x]);
if (reverse === undefined) reverse = y === "width" || y === "height"; // default to descending for lengths
if (reduce == null || reduce === false) continue; // disabled reducer
const X = findScaleChannel(channels, x) || (facetChannels && findScaleChannel(facetChannels, x));
const X = x === "fx" || x === "fy" ? reindexFacetChannel(facets, facetChannels[x]) : findScaleChannel(channels, x);
if (!X) throw new Error(`missing channel for scale: ${x}`);
const XV = X.value;
const [lo = 0, hi = Infinity] = isIterable(limit) ? limit : limit < 0 ? [limit] : [0, limit];
Expand Down Expand Up @@ -120,6 +120,21 @@ function findScaleChannel(channels, scale) {
}
}

// Facet channels are not affected by transforms; so, to compute the domain of a
// facet scale, we must first re-index the facet channel according to the
// transformed mark index. Note: mutates channel, but that should be safe here?
function reindexFacetChannel(facets, channel) {
const originalFacets = facets.original;
if (originalFacets === facets) return channel; // not transformed
const V1 = channel.value;
const V2 = (channel.value = []); // mutates channel!
for (let i = 0; i < originalFacets.length; ++i) {
const vi = V1[originalFacets[i][0]];
for (const j of facets[i]) V2[j] = vi;
}
return channel;
}

function difference(channels, k1, k2) {
const X1 = values(channels, k1);
const X2 = values(channels, k2);
Expand Down
4 changes: 3 additions & 1 deletion src/mark.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,11 @@ export class Mark {
initialize(facets, facetChannels) {
let data = arrayify(this.data);
if (facets === undefined && data != null) facets = [range(data)];
const originalFacets = facets;
if (this.transform != null) ({facets, data} = this.transform(data, facets)), (data = arrayify(data));
if (facets !== undefined) facets.original = originalFacets; // needed up read facetChannels
Copy link
Member Author

Choose a reason for hiding this comment

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

“needed up read facetChannels” 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

"needed to reindex the facet channel" (?)
or remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ha. I think I meant “needed to read facetChannels”.

const channels = Channels(this.channels, data);
if (this.sort != null) channelDomain(channels, facetChannels, data, this.sort); // mutates facetChannels!
if (this.sort != null) channelDomain(data, facets, channels, facetChannels, this.sort); // mutates facetChannels!
return {data, facets, channels};
}
filter(index, channels, values) {
Expand Down
262 changes: 262 additions & 0 deletions test/output/athletesSortFacet.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions test/plots/athletes-sort.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
import * as Plot from "@observablehq/plot";
import * as d3 from "d3";

export async function athletesSortFacet() {
const athletes = await d3.csv("data/athletes.csv", d3.autoType);
const female = (d) => d.sex === "female";
return Plot.plot({
marginLeft: 100,
marks: [
Plot.barX(athletes, Plot.groupZ({x: "mean"}, {x: female, fy: "sport", sort: {fy: "x"}})),
Plot.frame({anchor: "left", facet: "super"})
]
});
}

export async function athletesSortNullLimit() {
const athletes = await d3.csv("data/athletes.csv", d3.autoType);
return Plot.plot({
Expand Down