window skipNaN #996
Merged
window skipNaN #996
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbostock
requested changes
Jul 15, 2022
src/transforms/window.js
Outdated
| map(I, S, T) { | ||
| sum.map(I, S, T); | ||
| map(I, S, T, skipNaN) { | ||
| sum.map(I, S, T, skipNaN); |
There was a problem hiding this comment.
I think we won’t be able to write mean in terms of sum here because if we ignore NaN then the denominator should change (e.g., if k is 5 but one value in the window is NaN then the denominator should be 4 and the mean is the sum divided by 4). Otherwise we are saying that undefined values are zero (which could be valid but should be an alternate representation).
mbostock
reviewed
Jul 15, 2022
* generalize non-strict reducers * tweak * don’t coerce for mode * non-strict reducers * DRY * mode can be undefined, not NaN * fix first/last defined logic * don’t coerce for first and last * numbers cannot be null * shorter * fix comment Co-authored-by: Philippe Rivière <[email protected]>
mbostock
approved these changes
Jul 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re: #993 (comment)
There is a case for saying that having a window containing a NaN is the same thing as a window that is on an edge: the values "before the beginning" or "after the end" are unavailable and we either ignore them (and apply the reducer on valid values) or return NaN.
Put it differently, if you accept windows that "extend", then you probably don't care too much about NaNs and statistical consistency, and prefer a “best guess” result with no holes.
In that sense, we should support strict: true to mean "no extension, and return NaN if any value is NaN", and strict: false the opposite. (This being the default, we would have gone from a very strict concept of window transform to a very lenient one.)
Of course this is opinionated, but it's consistent with the notion that a time series (employment 1990-2017) is in itself a window in time.