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

make dims (previously sometimes called region) a keyword argument #25989

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Feb 10, 2018

Continues #25831, fixes #25501.

I still think the keyword argument is a better API, but after working on this I understand why @ararslan's opinion shifted. The basic issue is that reducing over dimensions generally requires a different algorithm and (in our case) gives a different type of result, so it's useful to dispatch on whether a dims argument is passed. Since we don't dispatch on keyword arguments, quite a few contortions are needed.

I found a total of 22 affected functions. Somebody who understands the stats functions better should review to make sure those changes make sense. There are also a few un-exported functions (covm, covzm, corm, corzm, reducedim!, mapreducedim!, maybe a few more) that take dim arguments, but I left those alone to save time. We might want to change them too though.

@JeffBezanson JeffBezanson added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation labels Feb 10, 2018
@JeffBezanson JeffBezanson added this to the 1.0 milestone Feb 10, 2018
@JeffBezanson JeffBezanson force-pushed the jb/dims-kwarg branch 3 times, most recently from 55980f5 to 96d45b4 Compare February 11, 2018 17:52
@ararslan
Copy link
Member

I resolved the conflicts and the CI failures are unrelated. Good to go?

@JeffBezanson
Copy link
Member Author

Thanks.

I'm still wondering about the un-exported stats functions. Do people use them enough that it's worth changing them to the new style?

@ararslan
Copy link
Member

They're used in StatsBase but that's the only place I've seen them. Since they aren't exported I guess we would consider them "internal" in some sense, so it seems okay to me not to change them. Presumably if they were used that often they would be exported.

I'd also be interested to hear what @andreasnoack's thoughts on that are.

@JeffBezanson JeffBezanson merged commit f32a9ce into master Feb 23, 2018
@JeffBezanson JeffBezanson deleted the jb/dims-kwarg branch February 23, 2018 00:41
mbauman added a commit that referenced this pull request Feb 23, 2018
Issue #25989 accidentally broke `std(rand(10))`.  This fixes it.
KristofferC pushed a commit that referenced this pull request Feb 24, 2018
Issue #25989 accidentally broke `std(rand(10))`.  This fixes it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make region a keyword argument for dimension reducing methods
2 participants