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

MAINT: deduplicate check_nonreorderable_axes #10309

Merged

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jan 2, 2018

Call was present in both branches of the if statement

Found while looking at #834

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with one small comment.

@@ -496,30 +487,29 @@ PyUFunc_ReduceWrapper(PyArrayObject *operand, PyArrayObject *out,
goto fail;
}

/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be moved one if-statement further up, so that it goes together with the sanity check on wheremask (and then one can just return NULL instead of goto fail).

Actually, it might even make more sense in ufunc_object, before we even get here (right after getting the identity, which reorderable depends on), so that one doesn't have to pass reorderable in at all, but that is perhaps too much of a change, and may be better left for when/if we unify reduction and single-axis ufuncs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points

@eric-wieser eric-wieser force-pushed the check_nonreorderable_axes-deduplicate branch from e6f4a2a to 1a09222 Compare January 3, 2018 14:27
@eric-wieser
Copy link
Member Author

Updated with a little more cleanup

@eric-wieser eric-wieser force-pushed the check_nonreorderable_axes-deduplicate branch from 1a09222 to 4693e68 Compare January 3, 2018 15:40
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice further cleanup; good to have the error raised in the main routine.

@mhvk mhvk merged commit e15c0a3 into numpy:master Jan 3, 2018
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.

None yet

3 participants