Skip to content

If an ordinal domain is specified, any corresponding values that are not in the domain should be considered undefined. #52

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

Closed
mbostock opened this issue Dec 15, 2020 · 2 comments · Fixed by #280
Labels
bug Something isn’t working

Comments

@mbostock
Copy link
Member

mbostock commented Dec 15, 2020

For example,

Plot.plot({
  x: {
    domain: "AEIOU"
  },
  y: {
    grid: true
  },
  marks: [
    Plot.barY(alphabet, {x: "letter", y: "frequency"}),
    Plot.ruleY([0])
  ]
})

Expected
Screen Shot 2020-12-15 at 2 24 48 PM

Actual
Screen Shot 2020-12-15 at 2 24 23 PM

@Fil
Copy link
Contributor

Fil commented Dec 16, 2020

If we want to generalize this issue, a scale that returns NaN for a datum should never have that datum pass the "filter" stage, and no element should be created in the selectAll().join.

But in the current model the scales are applied after the filter.

So the change might look like this (here I fixed only x, but the real fix would happen in all the Marks and for all the dimensions):

diff --git a/src/marks/bar.js b/src/marks/bar.js
index f38a589..09aa0e3 100644
--- a/src/marks/bar.js
+++ b/src/marks/bar.js
@@ -41,7 +41,9 @@ export class AbstractBar extends Mark {
   render(I, scales, channels, options) {
     const {color} = scales;
     const {z: Z, fill: F, stroke: S} = channels;
-    const index = filter(I, ...this._positions(channels), F, S);
+    let index = filter(I, ...this._positions(channels), F, S);
+    const x = Array.from(index, this._x(scales, channels, options));
+    index = filter(I, x);
     if (Z) index.sort((i, j) => ascending(Z[i], Z[j]));
     return create("svg:g")
         .call(applyIndirectStyles, this)
@@ -50,7 +52,7 @@ export class AbstractBar extends Mark {
           .data(index)
           .join("rect")
             .call(applyDirectStyles, this)
-            .attr("x", this._x(scales, channels, options))
+            .attr("x", i => x[i])
             .attr("width", this._width(scales, channels, options))
             .attr("y", this._y(scales, channels, options))
             .attr("height", this._height(scales, channels, options))

alternatively

diff --git a/src/marks/bar.js b/src/marks/bar.js
index f38a589..0822bf2 100644
--- a/src/marks/bar.js
+++ b/src/marks/bar.js
@@ -41,7 +41,8 @@ export class AbstractBar extends Mark {
   render(I, scales, channels, options) {
     const {color} = scales;
     const {z: Z, fill: F, stroke: S} = channels;
-    const index = filter(I, ...this._positions(channels), F, S);
+    const x = Array.from(I, this._x(scales, channels, options));
+    const index = filter(I, ...this._positions(channels), F, S, x);
     if (Z) index.sort((i, j) => ascending(Z[i], Z[j]));
     return create("svg:g")
         .call(applyIndirectStyles, this)
@@ -50,7 +51,7 @@ export class AbstractBar extends Mark {
           .data(index)
           .join("rect")
             .call(applyDirectStyles, this)
-            .attr("x", this._x(scales, channels, options))
+            .attr("x", i => x[i])
             .attr("width", this._width(scales, channels, options))
             .attr("y", this._y(scales, channels, options))
             .attr("height", this._height(scales, channels, options))

The second option makes a few more computations by applying _x to all indices, including those that would be rejected by the first filtering—but as a benefit the index i is true if the function to be called is (d,i) => something.

Systematically rejecting all elements for which whichever attribute would be computed as NaN (or null or undefined) probably makes sense?

@mbostock mbostock added the bug Something isn’t working label Feb 24, 2021
@mbostock mbostock added this to the Friends Preview milestone Feb 24, 2021
@Fil Fil self-assigned this Mar 3, 2021
@mbostock
Copy link
Member Author

mbostock commented Mar 8, 2021

Yes, that sounds right: we should pass values through the scale to determine whether the scaled value is defined.

@mbostock mbostock removed this from the Friends Preview milestone Mar 10, 2021
Fil added a commit that referenced this issue Mar 24, 2021
…llowing NaN, null and undefined as (ordinal) classes

fixes #52
fixes #45
Fil added a commit that referenced this issue Mar 24, 2021
…llowing NaN, null and undefined as (ordinal) classes

fixes #52
fixes #45
@Fil Fil mentioned this issue Mar 24, 2021
@Fil Fil removed their assignment Mar 26, 2021
Fil added a commit that referenced this issue Mar 27, 2021
…llowing NaN, null and undefined as (ordinal) classes

groups respect the domain option

fixes #52
fixes #45
fixes #255
supersedes #272
Fil added a commit that referenced this issue Mar 27, 2021
…llowing NaN, null and undefined as (ordinal) classes

groups respect the domain option

fixes #52
fixes #45
fixes #255
supersedes #271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working
Projects
None yet
2 participants