Skip to content

Adjust constraints on ddof for var_axis and std_axis #515

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

Merged
merged 3 commits into from
Nov 27, 2018

Conversation

jturner314
Copy link
Member

Requiring ddof to not be < 0 allows us to ignore weird edge cases. Allowing ddof to be equal to the length of the axis allows computation of the population variance (ddof = 0) for a zero-length axis and computation of the sample variance (ddof = 1) for an axis of length one. (The result in these cases is an array of NaNs.)

Note that this is a breaking change because of the new constraint that ddof must not be less than zero.

@LukeMathWalker
Copy link
Member

Given that we are doing a breaking change, it might be worth it to swap the operation order in the function body: computing (and eventually panicking) on ddof before doing the actual computation.
This could be achieved by asking A to implement FromPrimitive.

@jturner314 jturner314 force-pushed the change-var-std-bounds branch from 2a71d35 to a4fcf24 Compare October 26, 2018 16:12
Requiring `ddof` to not be < 0 allows us to ignore weird edge cases.
Allowing `ddof` to be equal to the length of the axis allows
computation of the population variance (`ddof` = 0) for a zero-length
axis and computation of the sample variance (`ddof` = 1) for an axis
of length one. (The result in these cases is an array of NaNs.)

Note that this is a breaking change because of the new constraint that
`ddof` must not be less than zero.
The documentation for the `Zero` and `One` traits says only that they
are the additive and multiplicative identities; it doesn't say
anything about converting an integer to a float by adding `One::one()`
to `Zero::zero()` repeatedly. Additionally, it's nice to panic early
instead of waiting until after the sum has been calculated.
@jturner314 jturner314 force-pushed the change-var-std-bounds branch from a4fcf24 to 4f06af5 Compare October 26, 2018 16:16
@jturner314
Copy link
Member Author

That's a good idea. I've added a new commit using FromPrimitive.

@bluss bluss merged commit e1a6db8 into rust-ndarray:master Nov 27, 2018
@jturner314 jturner314 deleted the change-var-std-bounds branch November 27, 2018 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants