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

stack handles negative zero #1967

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Jan 2, 2024

@mbostock mbostock requested a review from Fil January 2, 2024 18:43
@Fil
Copy link
Contributor

Fil commented Jan 2, 2024

🤔 We don't support -0 anywhere else, so this introduces an incongruity. For example, if the value we need to stack is the output of an aggregation with a "sum" reducer, we wouldn't be able to generate -0 (although the whole series might be sums of negative numbers).

As an alternative, I'd suggest adding any zeroes to yn if yp is zero. The patch can't be simpler:

-            else Y2[i] = Y1[i] = yp; // NaN or zero
+            else Y2[i] = Y1[i] = yp || yn; // NaN or zero

@Fil
Copy link
Contributor

Fil commented Jan 2, 2024

(Also re-reading this code I wonder why compare is in its own loop, and reverse is not. For readability, could we move compare just above reverse, or vice-versa?)

@Fil
Copy link
Contributor

Fil commented Jan 2, 2024

My (very unimaginative) unit test:

export async function stackZeroes() {
  const data = Array.from({length: 100}, d3.randomNormal.source(d3.randomLcg(42))()).map((value, i) => ({
    value: i % 7 === 0 ? 0 : value,
    x: Math.floor(i / 3),
    series: i % 3
  }));
  return Plot.plot({
    marks: [
      Plot.areaY(data, {x: "x", fill: "series", y: (d) => Math.min(d.value, 0)}),
      Plot.areaY(data, {x: "x", fill: "series", y: (d) => Math.max(d.value, 0)})
    ]
  });
}

@mbostock
Copy link
Member Author

mbostock commented Jan 2, 2024

The alternative you suggest is not desirable as it will cause (positive) zero values to swap to the bottom when all the other values are positive, rather than maintaining the input order.

The purpose of this change is simply to allow -0 to indicate a zero value that stacks with the other negative values. We don’t need to do anything else.

@Fil
Copy link
Contributor

Fil commented Jan 2, 2024

I've spent a moment trying to understand the situation you're describing, but I don't get it. Seems to me that (EDIT: with my suggested patch) zeroes in a stack of positives are properly stacked in input order, as are zeroes in a stack of negatives. This solves the original issue with two separate series min(V,0) and max(V, 0).

The "known unknown" case is when a zero arrives in a stack where we've already seen non-negatives and non-positives. In that case it goes arbitrarily to the top of the positive stack (yp); this might not be desirable and could (additionally) be explicitly routed with -0.

@mbostock
Copy link
Member Author

mbostock commented Jan 2, 2024

@Hvass-Labs says it doesn’t work with 0 as I linked in the OP #1963 (reply in thread). But seems like we need the actual data to confirm that this is the problem and not something else?

@mbostock
Copy link
Member Author

mbostock commented Jan 2, 2024

You can see the problem in the linked images:

image

The negative zero values jump up to the y = 0 baseline because they are erroneously considered positive, hence the need to use -1e-35 as the workaround.

@Fil
Copy link
Contributor

Fil commented Jan 2, 2024

I've edited my comment above to clarify that it relates to my suggested patch.

@mbostock
Copy link
Member Author

mbostock commented Jan 2, 2024

Conceptually, -0 is a negative number and hence should be stacked with the other negative values. This feels more explicit than having the treatment of negative zero depend on whether there’s any preceding positive value in the current stack.

Edit: I’m proposing that we use negative zero to indicate explicitly that you want the zero to stack with the negative values rather than the positive values.

Fil added a commit that referenced this pull request Jan 3, 2024
(alternative/complement to #1967)
@Fil Fil mentioned this pull request Jan 3, 2024
@Fil
Copy link
Contributor

Fil commented Jan 3, 2024

Agree that if the user passes -0 it is logical to stack it on yn. But should we depend on this peculiar value to answer the OP? It feels like an easter egg. (I understand it's a Number, but it feels like a special symbol that needs specific documentation and is not so easily generated).

I've pushed my suggestion to #1968 — with a funky test case.

But note that we could have both: stack a 0 value on the "active" side (yp || yn), and stack an explicit -0 on the negative side yn:

-            else Y2[i] = Y1[i] = yp; // NaN or zero
+            else Y2[i] = Y1[i] = 1 / y === -Infinity ? yn : yp || yn; // NaN or zero

I don't want to block this further, I just hope I've made my case :) Whatever you prefer, it will be good to include several test plots.

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

Why not, but see #1968 for an alternative/complementary suggestion (and unit test).

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.

2 participants