-
Notifications
You must be signed in to change notification settings - Fork 307
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
Mean #580
Mean #580
Conversation
The CI run on nightly seems to be failing for unrelated reasons (failing to install Rust itself). |
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.
The CI issue (due to rust-lang/rust#57488) is now fixed, so I re-ran CI.
I added a few comments. Additionally, if we add tests/numeric.rs
(which is a good idea IMO), we should also move the relevant tests in tests/array.rs
to it (e.g. sum_mean
, sum_mean_empty
, var_axis
, std_axis
, etc.).
Everything else looks good to me.
All done - I have moved all the numeric tests I could spot in |
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.
Looks good to me. Once this is merged, I think we should also change var_axis
and std_axis
to return an Option
so that they're consistent with mean_axis
. [Edit: They do require A: Float
, though, so this isn't strictly necessary.]
I am happy to do the changes to |
I'm not quite sure why I waited to merge this. (Maybe I was waiting on #577?) Anyway, let's go ahead and merge it. Thanks for your work on this @LukeMathWalker! Edit: I've rearranged the commit history to keep some commits separate but combine several small ones. |
Upstream
mean
fromndarray-stats
tondarray
.Breaking change on
mean_axis
: it returnsOption<Array<A, D:Smaller>>
now.