Skip to content

Conversation

@melange396
Copy link
Contributor

This will not plot null values at all, whereas before it was showing them as "zero" in the y dimension.

This effort is in furtherance of issue:

Comment on lines -722 to 724
for (let i = 0; i < data.length; i++) {
const y = value2y(dataset.getPointValue(i));
if (isNaN(y)) {
const ptVal = dataset.getPointValue(i);
if (isNaN(ptVal) || ptVal == null) {
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it was intended to prevent plotting null values in the data, but there were two major problems with the logic:

First, JavaScript's isNaN() doesnt quite determine if the object in question is a number or not; it determines if it is (or if it would be coerced to) the numeric value of NaN (a reserved word in the language). You might think that null is not a number, but when used in a numeric expression, null is "coerced" to the numeric value of zero. (Similarly, the string "1" is coerced to numeric 1 and string "0" to numeric 0. The string "xyz" would be coerced to NaN because it cannot be interpreted as a numeric value. For strange historical reasons and by convention, the empty string "" also becomes 0.) Thus, isNaN() is not sufficient to test for null, and that must be done explicitly.

isNaN(null)  == false;
isNaN(0)     == false;
isNaN("1")   == false;
isNaN("")    == false;
isNaN(NaN)   == true;
isNaN("xyz") == true;

Second, since null is coerced to 0 when used in a numeric expression, we must test a value for equality to null before it is run through such an expression... And it is transformed by arithmetic operations when passed to value2y(), so testing for null afterward is futile!

let a = null, b = null;
a += 0; // identity operation?
b *= 1; // identity operation?
console.log(a, b); // => "null null"

@melange396 melange396 requested review from RoniRos and dmytrotsko March 8, 2025 05:19
Copy link
Member

@RoniRos RoniRos left a comment

Choose a reason for hiding this comment

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

LGTM.

@RoniRos RoniRos merged commit f2567c6 into dev Mar 8, 2025
6 checks passed
@RoniRos RoniRos deleted the dont_plot_nulls branch March 8, 2025 13:32
This was referenced Mar 8, 2025
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.

3 participants