Skip to content

take() implementation #96

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

Closed
wants to merge 2 commits into from
Closed

take() implementation #96

wants to merge 2 commits into from

Conversation

honno
Copy link
Contributor

@honno honno commented Mar 28, 2023

Rudimentary take() implementations. Passes my Array API test_take at least (data-apis/array-api-tests#173, no means comprehensive for NumPy-propers semantics of take()), and un-xfailing existing tests didn't uncover anything—I imagine however importing more tests we'll find a bug or two for sure though.

It's kinda awkward to implement ndarray.take() given the redundancies of handling the kwargs in _wrapper.take() too (which cannot be used due to circular import), so left it not implemented...

Hopefully this is how I should be implementing things now? I really like @normalizer!

@honno honno requested a review from ev-br March 28, 2023 10:12
Copy link
Collaborator

@ev-br ev-br 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!

Let's hold on merging this for a while, until the dust settles in gh-93 and gh-94. After that, there will be a small conflict (_wrapper.py -> _funcs.py, no _impl.py), but this will be easy to take care of at merge time.

# [[1, 1], [0, 1]], [[1, 0], [0, 0]]
# >>> torch.unique(torch.as_tensor(x), dim=2)
# [[1, 1], [1, 0]], [[0, 1], [0, 0]]
#
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this something we need to fix in our unique?

The Notes section in https://numpy.org/doc/stable/reference/generated/numpy.unique.html is probably relevant, if somewhat obscure.

WDYT @lezcano ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(unique against axis gave me a headache so I had left this xfail'd and documented incase you/Mario had an immediate answer 😅)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uf. This looks like a bit of a pain to implement. I'd say we don't touch it and leave it xfailed.

@ev-br
Copy link
Collaborator

ev-br commented Mar 31, 2023

Rebased and merged in gh-98

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.

3 participants