Skip to content

fix test_float_to_int_conversion_nonfinite for NumPy 2#138131

Closed
haifeng-jin wants to merge 2 commits intopytorch:mainfrom
haifeng-jin:test_tensor_creation_ops
Closed

fix test_float_to_int_conversion_nonfinite for NumPy 2#138131
haifeng-jin wants to merge 2 commits intopytorch:mainfrom
haifeng-jin:test_tensor_creation_ops

Conversation

@haifeng-jin
Copy link
Collaborator

@haifeng-jin haifeng-jin commented Oct 16, 2024

Related to #107302

We saw test_float_to_int_conversion_nonfinite failed as we upgrade to NumPy 2.

It is caused by the undefined behavior of numpy casting inf, -inf and nan from np.float32 to other dtypes.
The test is using NumPy as reference for the ground truth. (see line 1013-1015)
However, these behaviors are undefined in NumPy.
If you do np.array([float("inf")]).astype(np.uint8, casting="safe"), it results in an error TypeError: Cannot cast array data from dtype('float64') to dtype('uint8') according to the rule 'safe'.
The undefined behaviors are always subject to change.

This PR address this issue by passing concrete values as the ground truth references.
In the future, even NumPy changes its behavior the test would still remain stable.

cc @mruberry @rgommers @kiukchung

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 16, 2024

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 37d20eb with merge base 5b1c67c (image):
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Oct 16, 2024
@drisspg drisspg requested a review from albanD October 18, 2024 02:09
@drisspg drisspg added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 18, 2024
@haifeng-jin
Copy link
Collaborator Author

haifeng-jin commented Oct 30, 2024

Hi @albanD,
would you mind taking a look at this PR?
Or just reassign it if you are busy?

@drisspg
Would you mind reassign it if @albanD is busy?

We are blocked by this to enable the NumPy 2 CI tests.
Thanks!

@drisspg
Copy link
Contributor

drisspg commented Oct 30, 2024

Looks good

@drisspg drisspg added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Oct 30, 2024
@haifeng-jin
Copy link
Collaborator Author

@drisspg Thank you!
Is this PR ready to merge?

@haifeng-jin
Copy link
Collaborator Author

@drisspg friendly ping again : )

@kiukchung
Copy link
Collaborator

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 8, 2024
@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command
For more information see pytorch-bot wiki.

@haifeng-jin
Copy link
Collaborator Author

@pytorchbot rebase

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 11, 2024

You don't have permissions to rebase this PR since you are a first time contributor. If you think this is a mistake, please contact PyTorch Dev Infra.

@haifeng-jin haifeng-jin force-pushed the test_tensor_creation_ops branch from ca94ee4 to 81b7b5a Compare November 11, 2024 19:09
@drisspg
Copy link
Contributor

drisspg commented Nov 12, 2024

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot pytorchmergebot force-pushed the test_tensor_creation_ops branch from 81b7b5a to 9454c16 Compare November 12, 2024 03:05
@seemethere
Copy link
Member

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-py3-arm64 / test (default, 1, 3, macos-m1-stable)

Details for Dev Infra team Raised by workflow job

@haifeng-jin haifeng-jin force-pushed the test_tensor_creation_ops branch from 04f3899 to 37d20eb Compare November 13, 2024 05:52
@haifeng-jin
Copy link
Collaborator Author

Skip macos-arm64 for the test since the torch ops return a different result than other OSs.
It seems to be a long existing issue. We did not catch it just because we did not pass the reference values for the tests.

See more details: #38752

@kiukchung
Copy link
Collaborator

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Not merging any PRs at the moment because there is a merge blocking https://github.com/pytorch/pytorch/labels/ci:%20sev issue open at:
#140631

Details for Dev Infra team Raised by workflow job

@izaitsevfb
Copy link
Contributor

@pytorchbot merge

apologies, this SEV should not be merge blocking

@pytorchmergebot
Copy link
Collaborator

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

pobin6 pushed a commit to pobin6/pytorch that referenced this pull request Dec 5, 2024
Related to pytorch#107302

We saw `test_float_to_int_conversion_nonfinite` failed as we upgrade to NumPy 2.

It is caused by the undefined behavior of `numpy` casting `inf`, `-inf` and `nan` from `np.float32` to other dtypes.
The test is using NumPy as reference for the ground truth. (see line 1013-1015)
However, these behaviors are undefined in NumPy.
If you do `np.array([float("inf")]).astype(np.uint8, casting="safe")`, it results in an error `TypeError: Cannot cast array data from dtype('float64') to dtype('uint8') according to the rule 'safe'`.
The undefined behaviors are always subject to change.

This PR address this issue by passing concrete values as the ground truth references.
In the future, even NumPy changes its behavior the test would still remain stable.

Pull Request resolved: pytorch#138131
Approved by: https://github.com/drisspg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: numpy Related to numpy support, and also numpy compatibility of our operators open source topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants