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

Add support for axis keyword argument to np.argmin() #7129

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

itamarst
Copy link
Contributor

@itamarst itamarst commented Jun 21, 2021

Note to reviewers: this PR is actually much simpler than the line count would suggest, for the most part it's just very minor refactoring (refactoring internal functions so they're top-level functions) and reuses a lot of existing code from #6928.

I have added inline comments noting where code is unchanged and merely moved.

min_idx = 0
if _is_nat(min_value):
return min_idx
@register_jitable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function's code is unchanged, it's simply moved to the top-level whereas previously it was internal to array_argmin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed, thanks.

min_idx = idx
idx += 1
return min_idx
@register_jitable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function's code is unchanged, it's simply moved to the top-level whereas previously it was internal to array_argmin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed, thanks.


@register_jitable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function's code is unchanged, it's simply moved to the top-level whereas previously it was internal to array_argmin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed, thanks.

@@ -771,6 +786,50 @@ def array_argmax_impl_generic(arry):
return max_idx


def build_argmax_or_argmin_with_axis_impl(arr, axis, flatten_impl):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function's code is unchanged, it's simply moved to the top-level (whereas previously it was internal to array_argmax) so it can be used by both argmin and argmax implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed. Thanks.

@@ -785,42 +844,9 @@ def array_argmax(arr, axis=None):
def array_argmax_impl(arr, axis=None):
return flatten_impl(arr)
else:
_check_is_integer(axis, "axis")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code now lives in build_argmax_or_argmin_with_axis_impl, unchanged otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed. Thanks.

@itamarst itamarst marked this pull request as ready for review June 21, 2021 14:41
@itamarst itamarst changed the title DRAFT: Add support for axis keyword argument to np.argmin() Add support for axis keyword argument to np.argmin() Jun 21, 2021
@stuartarchibald stuartarchibald added this to the PR Backlog milestone Jun 21, 2021
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, looks good! Thanks also for leaving notes to help with the review, they are much appreciated.

min_idx = 0
if _is_nat(min_value):
return min_idx
@register_jitable
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed, thanks.

min_idx = idx
idx += 1
return min_idx
@register_jitable
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed, thanks.


@register_jitable
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed, thanks.

@@ -771,6 +786,50 @@ def array_argmax_impl_generic(arry):
return max_idx


def build_argmax_or_argmin_with_axis_impl(arr, axis, flatten_impl):
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed. Thanks.

@@ -785,42 +844,9 @@ def array_argmax(arr, axis=None):
def array_argmax_impl(arr, axis=None):
return flatten_impl(arr)
else:
_check_is_integer(axis, "axis")
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed. Thanks.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Aug 3, 2021
@sklam sklam merged commit 88c1003 into numba:master Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants