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

the map transform should respect *z* series specified as {value: z} #2272

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 19, 2025

Currently when z is specified as stroke: {value: "series"}, it is ignored by the window (map) transform and we get the incorrect chart.

correct incorrect
Image Image

@Fil Fil changed the title the window transform should respect *z* series the window transform should respect *z* series defined as an object Jan 19, 2025
@Fil Fil changed the title the window transform should respect *z* series defined as an object the window transform should respect *z* series specified as {value: z} Jan 19, 2025
@Fil Fil changed the title the window transform should respect *z* series specified as {value: z} the map transform should respect *z* series specified as {value: z} Jan 19, 2025
@Fil
Copy link
Contributor Author

Fil commented Jan 19, 2025

The discrepancy comes from the fact that the specification {value: "type"} is analyzed in mark.js to create the marks object’s this.channels; but in the map transform we don't make that same analysis and pass {value: "type"}directly to valueof.

The fix I pushed above fixes the situation for the map transform, but the same problem might appear in other places, and it might be better to fix it at the level of valueof. The patch in that case would be:

--- a/src/options.js
+++ b/src/options.js
@@ -32,6 +32,7 @@ function isBigIntType(type) {
 export const reindex = Symbol("reindex");

 export function valueof(data, value, type) {
+  if (isObject(value) && value.value !== undefined) ({value} = value);
   const valueType = typeof value;

Both patches pass all tests, but it only means we don't have a test for other places where z might be computed separately from channels (probably in the stack transform at least … this needs a bit more work to make sure we get the whole picture and add the necessary tests).

In fact, I'm wondering why we compute z separately as a valueof, and not take it from the mark's channels.

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.

1 participant