-
Notifications
You must be signed in to change notification settings - Fork 187
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
Raise an error when a reducer expects numeric input but is given non-numeric input #487
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll chip in to help with these.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a slightly different assertion than what we do elsewhere, e.g.:
Lines 104 to 108 in b1885ad
// If any channel is ordinal or temporal, it takes priority. | |
const values = channels.map(({value}) => value).filter(value => value !== undefined); | |
if (values.some(isOrdinal)) return asOrdinalType(key); | |
if (values.some(isTemporal)) return "utc"; | |
return "linear"; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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