Skip to content

Fix torch._numpy to match NumPy when empty ellipsis causes advanced indexing separation#158297

Closed
manuelcandales wants to merge 4 commits intomainfrom
manuel/np_empty_ellipsis
Closed

Fix torch._numpy to match NumPy when empty ellipsis causes advanced indexing separation#158297
manuelcandales wants to merge 4 commits intomainfrom
manuel/np_empty_ellipsis

Conversation

@manuelcandales
Copy link
Contributor

@manuelcandales manuelcandales commented Jul 14, 2025 •
edited
Loading

Fixes #141563

In NumPy, an ellipsis always acts as a separator between advanced indices, even when the ellipsis doesn't actually match any dimensions. In PyTorch an empty ellipsis doesn't cause a separation. This leads to differing behavior between Numpy and PyTorch in this edge case.

This difference in behavior leads to a bug when using torch.compile:

>>> import numpy as np
>>> f = lambda x: x[:,(0,1),...,(0,1)].shape
>>> a = np.ones((3, 4, 5))
>>> f(a)
(2, 3)
>>> torch.compile(f)(a)
(3, 2)

Similarly to #157676, this PR doesn't change PyTorch's behavior, but it fixes the translation layer, ensuring torch._numpy compatibility with NumPy. I am marking this PR as fixing #141563, even though PyTorch behavior isn't modified.

Notice that there are still some other bugs in PyTorch's advanced indexing, that need to be fixed (mainly regarding proper accounting of dimensions when multidimensional boolean masks are present). But those need to be fixed at the ATen operator level. Examples:

cc @mruberry @rgommers

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 14, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/158297

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit db97f27 with merge base cf3247b (image):

UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@manuelcandales manuelcandales added topic: not user facing topic category module: numpy Related to numpy support, and also numpy compatibility of our operators labels Jul 14, 2025
@manuelcandales manuelcandales requested a review from soumith July 14, 2025 23:46
@manuelcandales manuelcandales force-pushed the manuel/np_empty_ellipsis branch from 1019f97 to cd44f8f Compare July 15, 2025 01:27
soumith
soumith approved these changes Jul 15, 2025
View reviewed changes
Copy link
Collaborator

@soumith soumith left a comment

Choose a reason for hiding this comment

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

CI needs to be fixed but the patch looks good

@seemethere
Copy link
Member

seemethere commented Jul 15, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 15, 2025

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 15, 2025

Successfully rebased manuel/np_empty_ellipsis onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout manuel/np_empty_ellipsis && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the manuel/np_empty_ellipsis branch from cd44f8f to 1db646c Compare July 15, 2025 16:24
@manuelcandales
Copy link
Contributor Author

manuelcandales commented Jul 15, 2025

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 15, 2025

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 15, 2025

Successfully rebased manuel/np_empty_ellipsis onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout manuel/np_empty_ellipsis && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the manuel/np_empty_ellipsis branch from 1db646c to 5024183 Compare July 15, 2025 22:08
@manuelcandales manuelcandales force-pushed the manuel/np_empty_ellipsis branch from 5024183 to db97f27 Compare July 16, 2025 01:20
@manuelcandales
Copy link
Contributor Author

manuelcandales commented Jul 16, 2025

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 16, 2025
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Jul 16, 2025

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@github-actions github-actions bot deleted the manuel/np_empty_ellipsis branch August 16, 2025 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soumith soumith soumith approved these changes

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: numpy Related to numpy support, and also numpy compatibility of our operators topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Advanced indexing with empty ellipsis (discrepency to numpy)

4 participants

@manuelcandales @seemethere @pytorchmergebot @soumith