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

[Feat] Add support for index tricks in autograd numpy #639

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fjosw
Copy link
Collaborator

@fjosw fjosw commented Sep 8, 2024

As requested in #623 I had a look at adding support for np.mgrid and a few other index tricks. Just importing them in numpy.__init__ makes them available but I'm not sure how much value that actually adds. What is your opinion @agriyakhetarpal ?

@fjosw fjosw mentioned this pull request Sep 8, 2024
@j-towns
Copy link
Collaborator

j-towns commented Sep 9, 2024

The functions

diag_indices
diag_indices_from
fill_diagonal
ndenumerate
ndindex
ravel_multi_index
unravel_index

are all already there, so we don't need to add them. They are wrapped because they are callable, which happens here.

The only ones we potentially need to add are

index_exp
mgrid
ogrid
s_

They don't get wrapped because they accept their arguments as indices. It should be possible to wrap them in a similar way to r_ and c_, which are done here. If you want to try implementing that I'm happy to review a pr.

Probably best to do it in autograd/numpy/numpy_wrapper.py, not in autograd/numpy/__init__.py.

@agriyakhetarpal
Copy link
Collaborator

I'm sorry; I just realised I had noticed this and not responded earlier. I do not have much to say here – I would echo Jamie's suggested changes. It would be nice to have a few test cases as well.

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