testing: dont skip test_ops suite for operators testing against scipy #54186
Conversation
💊 CI failures summary and remediationsAs of commit 1f93c0c (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
kshitij12345
force-pushed
the
testing/scipy-opinfo-test
branch
from
2ad092a to
b85827f
Compare
Codecov Report
@@ Coverage Diff @@
## master #54186 +/- ##
===========================================
+ Coverage 40.93% 77.27% +36.33%
===========================================
Files 3 1891 +1888
Lines 149 185544 +185395
===========================================
+ Hits 61 143374 +143313
- Misses 88 42170 +42082 |
| if TEST_SCIPY: | ||
| import scipy | ||
|
|
||
| reference_filtered_ops = list(filter(lambda op: op.ref is not _NOTHING, unary_ufuncs)) |
There was a problem hiding this comment.
Add a comment explaining this filter
| return samples | ||
|
|
||
|
|
||
| def scipy_reference_wrapper(ref): |
There was a problem hiding this comment.
Clarify how and when to use this.
"If you're using a SciPy op as a reference, then pass it wrapped in a lambda to this function, which will either return the lambda or, if SciPy is unavailable, _NOTHING. Wrapping the operation in a lambda will prevent the Python interpreter from attempting to discover the operation, which will fail if SciPy is unavailable. See the OpInfo for digamma for an example."
| assert_autodiffed=True, | ||
| test_complex_grad=False), # Reference: https://github.com/pytorch/pytorch/issues/48552 | ||
| UnaryUfuncInfo('digamma', | ||
| ref=scipy_reference_wrapper(lambda x: scipy.special.digamma(x)), |
There was a problem hiding this comment.
This lambda is perfect, but if this PR does want to cite it as an example then:
lambda *args, **kwargs: scipy.special.digamma(*args, **kwargs)
There was a problem hiding this comment.
Have only updated the lambda for digamma (being the example).
Have kept other lambda's more closer to their original signature
| # In some cases, output is NaN (for input close to | ||
| # negative integers) especially due to reduced precision | ||
| # in float16 and NaN's can't be tested for equality. | ||
| SkipInfo('TestCommon', 'test_variant_consistency_jit', |
There was a problem hiding this comment.
This skip is not needed but a new skip may be
There was a problem hiding this comment.
Have pulled the latest master and just update the ref arguments with wrapped. Kept the other stuff as is in the master.
There was a problem hiding this comment.
Nice work, @kshitij12345. I made a few suggestions. The biggest thing is that this PR will need a rebase to account for the latest OpInfo changes. Once that happens I'll prioritize its landing since it's a larger PR.
|
Have mostly addressed the reviews. Hopefully should be good to go. PTAL :) |
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
4 participants
Fixes #54152