Skip to content
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

Added implementation of var and std methods for ArrayBase #790

Merged
merged 5 commits into from
Jan 8, 2021

Conversation

kdubovikov
Copy link
Contributor

I have found that ArrayBase is missing single-dimensional implementations for var and std. While you can use var_axis(Axis(0), 1.), it can become verbose and unnecessary in code with lots of single-dimensional arrays. I think sum_axis and mean_axis have sum and mean for this reason too.

Please consider merging this pull request.

@kdubovikov
Copy link
Contributor Author

I see that the build fails at cargo clippy check un an unrelated file. Unsure if I need to fix that.

@bluss
Copy link
Member

bluss commented Apr 18, 2020

Thanks! Looks similar to #753 I don't mind whichever gets merged first

@ZuseZ4
Copy link
Contributor

ZuseZ4 commented Nov 12, 2020

@jturner314 and @bluss I think it makes sense to merge this?
If we merge #848 first and rerun Travis here it shouldn't fail anymore. Just to be sure.

@kdubovikov
Copy link
Contributor Author

@jturner314 , @bluss , @ZuseZ4 anything else I should do to help with the merge?

@bluss
Copy link
Member

bluss commented Jan 7, 2021

Looks great. I've rebased to current master so that tests will run properly with new CI. Given the commit history, I'd merge this with squash to clean it up a bit

@bluss
Copy link
Member

bluss commented Jan 8, 2021

Thanks!

@bluss bluss merged commit 9758af7 into rust-ndarray:master Jan 8, 2021
@bluss
Copy link
Member

bluss commented Jan 8, 2021

cc @xd009642 Your comment would actually be interesting - w.r.t overlapping functionality with ndarray-stats. I don't think ndarray needs to include more of this, also unsure how we ended up here. Moving this method to ndarray-stats wouldn't be wrong IMO.

@xd009642
Copy link
Contributor

xd009642 commented Jan 9, 2021

Yeah I wouldn't be opposed to moving this functionality to ndarray-stats and then deprecating here

@bluss
Copy link
Member

bluss commented Jan 10, 2021

Oh so we ended up with var and std here - because we already had them in the axis variants here since long ago. It's hard to draw the line what's so basic that it could stay here - but from the summary functions I guess only .sum() is the so basic one, the rest could really move to stats.

@bluss
Copy link
Member

bluss commented Jan 10, 2021

I'll make a PR to improve the array traversal here - for performance. Fixed in 225d967

@kdubovikov kdubovikov deleted the feature/var branch January 18, 2021 11:12
bluss added a commit that referenced this pull request Mar 28, 2021
PR #790 was forgotten in release note (because it was integrated without
a merge commit, I missed it).
@bluss bluss added this to the 0.15.0 milestone Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants