Raise an error when a reducer expects numeric input but is given non-numeric input #487
Conversation
src/mark.js
Outdated
| function isNumeric(values) { | ||
| for (const value of values) { | ||
| if (value == null) continue; | ||
| return !isNaN(+value); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a slightly different assertion than what we do elsewhere, e.g.:
Lines 104 to 108 in b1885ad
I think what we’re saying here is whether the values are “weakly” numeric, i.e., whether they can be treated as numbers (in this context). So renaming this to isWeaklyNumeric is probably good.
Also, I think we want to treat strict NaN as numeric in this context, since the goal here is to detect things that are not numbers (such as strings), not to check that the numbers themselves are valid. So, one possibility:
| function isNumeric(values) { | |
| for (const value of values) { | |
| if (value == null) continue; | |
| return !isNaN(+value); | |
| } | |
| } | |
| function isWeaklyNumeric(values) { | |
| for (const value of values) { | |
| if (value == null) continue; | |
| if (typeof value === "number") return true; // note: includes NaN! | |
| return !isNaN(+value); | |
| } | |
| } |
src/mark.js
Outdated
| export function checkNumeric(S) { | ||
| if (!isNumeric(S)) throw new Error("the reducer expects numeric input"); | ||
| } |
There was a problem hiding this comment.
This error message is too specific (referring to a reducer here). I’ll think about this…
src/transforms/group.js
Outdated
| @@ -170,9 +170,10 @@ function reduceFunction(f) { | |||
| }; | |||
| } | |||
|
|
|||
| function reduceAccessor(f) { | |||
| function reduceAccessor(f, check) { | |||
There was a problem hiding this comment.
I think I’d prefer a separate reduceNumericAccessor here rather than an argument.
|
related #751 |
4ff6504
to
d88610b
Compare
|
I've rewritten this PR against main and following your comments. One thing to note is that we run the test on each call to the reducer. It will usually be fast (returning immediately), so there is no need to memoize, but there might be weird cases (large tables full of null values) where it's not. |
Add tests for the normalize transform
closes #473