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
S390x complex division #108516
S390x complex division #108516
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/108516
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit c524f2f with merge base c6f435b (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
So what's the plan for the tests? |
For now it is needed to be figured out how they should actually be working on x86. |
I think it's definitely possible we did it wrong on X86. Let us know what your analysis concludes. |
I took one more look, but I don't know where the error is. In the end, it's a matter of precision, and different tests expect different precision. I'd like to have s390x behave similar to x86 for now. Also, I'll split out the test update into separate PR since it might break CI. |
dc654fe
to
033359f
Compare
Adopt algorithm from AVX2 implementation. This change fixes test test_complex_div_underflow_overflow_cpu_complex128 from test/test_binary_ufuncs.py At the same time it breaks some of Arithmetics/*.Division tests from vec_test_all_types_ZVECTOR, but it's also broken on AVX2 and AVX512.
033359f
to
be8267b
Compare
I've found that there was update for division algorithms in #93277, but tests were not updated. I did the updating. Now it works for s390x, but it still doesn't work for x86_64 unfortunately. The issue is in those 2 lines:
Function |
well, if you want to land this for S390x, it's fine to just keep the old x86 impl and ifdef this only |
Division algorithms were updated in pytorch#93277 but tests were not.
be8267b
to
c524f2f
Compare
@ezyang, like this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sure
@pytorchbot merge |
Merge failedReason: This PR needs a If not, please add the To add a label, you can comment to pytorchbot, for example For more information, see Details for Dev Infra teamRaised by workflow job |
@pytorchbot merge |
Merge startedYour 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 |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
@pytorchbot merge |
Merge startedYour 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 |
Adopt algorithm from AVX2 implementation. This change fixes test test_complex_div_underflow_overflow_cpu_complex128 from test/test_binary_ufuncs.py At the same time it breaks some of Arithmetics/*.Division tests from vec_test_all_types_ZVECTOR, but it's also broken on AVX2 and AVX512. Pull Request resolved: pytorch#108516 Approved by: https://github.com/ezyang
Adopt algorithm from AVX2 implementation.
This change fixes test test_complex_div_underflow_overflow_cpu_complex128
from test/test_binary_ufuncs.py
At the same time it breaks some of Arithmetics/*.Division tests
from vec_test_all_types_ZVECTOR,
but it's also broken on AVX2 and AVX512.
cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10