Extends OpInfo architecture with reference inputs, adds them for elementwise binary operators#74280
Closed
Extends OpInfo architecture with reference inputs, adds them for elementwise binary operators#74280
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
|
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 9be5c06 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
ngimel
reviewed
Mar 16, 2022
| l = sample.input | ||
| r = sample.args[0] | ||
|
|
||
| def to_numpy(x): |
Collaborator
There was a problem hiding this comment.
why don't you use SampleInput.numpy()? (SampleInput.numpy doesn't handle bfloat16 though)
Collaborator
There was a problem hiding this comment.
+1 on using SampleInput.numpy() here. It probably also should handle bfloat16 so we can use it as general method for reference tests.
Collaborator
Author
There was a problem hiding this comment.
Sure; I updated the SampleInput class's .numpy() method instead
| x = torch.tensor((), dtype=dtype, device=device) | ||
| else: | ||
| if dtype.is_floating_point or dtype.is_complex: | ||
| # work around torch.randn not being implemented for bfloat16 |
Collaborator
There was a problem hiding this comment.
randn is implemented for bfloat16
Collaborator
Author
There was a problem hiding this comment.
Probably not worth updating this legacy function?
| # TODO: update make_tensor to support extremal additions and remove this in favor of make_tensor | ||
| def _generate_input(self, shape, dtype, device, with_extremal): | ||
| if shape == (): | ||
| x = torch.tensor((), dtype=dtype, device=device) |
Collaborator
There was a problem hiding this comment.
it's extremely conterintuitive to produce 0-element 1d tensor for shape=(), was scalar meant here?
Collaborator
Author
There was a problem hiding this comment.
This is just copy-pasting the same function from earlier in the file and moving it closer to where it's used because it's legacy nonsense
Comment on lines
+2433
to
+2434
| x = torch.zeros(shape, dtype=dtype, device=device) | ||
| x[torch.randn(*shape) > 0.5] = True |
Collaborator
There was a problem hiding this comment.
Suggested change
| x = torch.zeros(shape, dtype=dtype, device=device) | |
| x[torch.randn(*shape) > 0.5] = True | |
| x = torch.randint(2, shape, dtype=dtype, device=device) |
Collaborator
There was a problem hiding this comment.
make_tensor also handles this:
Suggested change
| x = torch.zeros(shape, dtype=dtype, device=device) | |
| x[torch.randn(*shape) > 0.5] = True | |
| x = make_tensor(shape, dtype=dtype, device=device) |
Collaborator
Author
There was a problem hiding this comment.
Won't fix for this PR because this "change" is just moving a pre-existing function that's already marked as needing deletion
| self.assertEqual(x / s, x / y) | ||
| self.assertEqual(s / x, y / x) | ||
|
|
||
| # TODO: update make_tensor to support extremal additions and remove this in favor of make_tensor |
pmeier
reviewed
Mar 16, 2022
Collaborator
pmeier
left a comment
There was a problem hiding this comment.
I have a few nits and questions inline. Otherwise LGTM. Still, the PR is massive so I'm not confident that I found everything.
| l = sample.input | ||
| r = sample.args[0] | ||
|
|
||
| def to_numpy(x): |
Collaborator
There was a problem hiding this comment.
+1 on using SampleInput.numpy() here. It probably also should handle bfloat16 so we can use it as general method for reference tests.
Comment on lines
+2433
to
+2434
| x = torch.zeros(shape, dtype=dtype, device=device) | ||
| x[torch.randn(*shape) > 0.5] = True |
Collaborator
There was a problem hiding this comment.
make_tensor also handles this:
Suggested change
| x = torch.zeros(shape, dtype=dtype, device=device) | |
| x[torch.randn(*shape) > 0.5] = True | |
| x = make_tensor(shape, dtype=dtype, device=device) |
Comment on lines
+10382
to
+10383
| DecorateInfo(unittest.expectedFailure, 'TestBinaryUfuncs', 'test_type_promotion', device_type='cuda'), | ||
| )), |
Collaborator
There was a problem hiding this comment.
Shouldn't this skip only be for complex64 rather than for all dtypes?
Collaborator
Author
There was a problem hiding this comment.
test_type_promotion isn't instantiated for different dtypes, so no (although your question makes perfect sense)
| rhs = make_arg(shape_rhs, **op.rhs_make_tensor_kwargs) | ||
| broadcasts_input = (shape_lhs != torch.broadcast_shapes(shape_lhs, shape_rhs)) | ||
|
|
||
| sample_kwargs = kwargs.get('sample_kwargs', {}) |
Collaborator
There was a problem hiding this comment.
It seems kwargs is only used to extract sample_kwargs out of it. Maybe we can use **sample_kwargs directly?
Collaborator
Author
There was a problem hiding this comment.
I think you're suggesting using **kwargs directly instead of extracting sample_kwargs?
Yes I agree. This is a unnecessary refinement that would be useful if an OpInfo wanted separate kwargs for the generically generated elementwise binary samples vs. the bespoke ones, but that case doesn't exist today and may never exist. I simplified it.
| DecorateInfo(unittest.skip('Skipped!'), 'TestBinaryUfuncs'), | ||
| ), | ||
| sample_inputs_func=sample_inputs_igamma_igammac), | ||
| # TODO: FIXME, ideally by implemented grad for both inputs |
Collaborator
There was a problem hiding this comment.
Just marking since this is a large PR and we might forget otherwise.
| DecorateInfo(unittest.skip('Skipped!'), 'TestBinaryUfuncs'), | ||
| ), | ||
| sample_inputs_func=sample_inputs_igamma_igammac), | ||
| # TODO: FIXME, ideally by implementing grad for both inputs |
ngimel
approved these changes
Mar 16, 2022
| @@ -692,6 +718,7 @@ def __init__(self, | |||
|
|
|||
| # We run the sampling functions without tracking the gradiends of the creation of inputs | |||
| self.sample_inputs_func = torch.no_grad()(sample_inputs_func) | |||
| self.reference_inputs_func = None if reference_inputs_func is None else torch.no_grad()(reference_inputs_func) | |||
Collaborator
Author
There was a problem hiding this comment.
Consistency with sample_inputs_func, which is also run in a no_grad() context to prevent the tensors used in SampleInputs, which are often constructed using a few operations, from having a grad history
| if self.reference_inputs_func is None: | ||
| return self.sample_inputs_func(self, device, dtype, requires_grad, **kwargs) | ||
|
|
||
| if 'include_conjugated_inputs' in kwargs and kwargs.get('include_conjugated_inputs'): |
Collaborator
There was a problem hiding this comment.
We don't want to do ref testing on conjugated inputs?
Collaborator
Author
There was a problem hiding this comment.
It just seemed kinda extra. There's still a test that conjugated sample inputs work. How likely is it that conjugated sample inputs work but conjugated inputs don't work on an expanded set of tensors with different values?
Collaborator
Author
|
@pytorchbot merge this please |
Contributor
|
Hey @mruberry. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 21, 2022
…entwise binary operators (#74280) Summary: This PR extends our OpInfo test architecture with "reference inputs," an optional expansion of typical sample inputs that allows for more thorough testing. Currently only the elementwise binary operations implement an extended set of reference inputs. This PR also cleans up some smaller OpInfo-related issues, including several bugs, and it identified #74279. A reference inputs function can be specified for an OpInfo by filling in its "reference_inputs_func" metadata. If this is done it's recommended that the reference inputs function first call the sample inputs function, then produce additional sample inputs. See `reference_inputs_elementwise_binary` for an example of this pattern. In addition to implementing reference inputs for the elementwise binary operations, this PR improves their consistency and simplifies how their metadata is represented. The great majority now use a generic sample input function, and those that want extensions start by calling the generic sample input function and then adding additional samples. This removes many older sample input functions. The BinaryUfuncInfo subclass also now allows specifying scalar support more precisely, and reference inputs and error inputs are generated based on this metadata to ensure it's correct. cc kshitij12345 pmeier zou3519 Chillee Pull Request resolved: #74280 Approved by: https://github.com/ngimel Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/0aa3c39e5f296dd0871d0f849e295d3b7644ff2e Reviewed By: seemethere, osalpekar Differential Revision: D35013032 fbshipit-source-id: b47c40bc34467b8043a8a8cb26fe7c9ef2c6e0f4
shahofblah
pushed a commit
that referenced
this pull request
Mar 25, 2022
…entwise binary operators This PR extends our OpInfo test architecture with "reference inputs," an optional expansion of typical sample inputs that allows for more thorough testing. Currently only the elementwise binary operations implement an extended set of reference inputs. This PR also cleans up some smaller OpInfo-related issues, including several bugs, and it identified #74279. A reference inputs function can be specified for an OpInfo by filling in its "reference_inputs_func" metadata. If this is done it's recommended that the reference inputs function first call the sample inputs function, then produce additional sample inputs. See `reference_inputs_elementwise_binary` for an example of this pattern. In addition to implementing reference inputs for the elementwise binary operations, this PR improves their consistency and simplifies how their metadata is represented. The great majority now use a generic sample input function, and those that want extensions start by calling the generic sample input function and then adding additional samples. This removes many older sample input functions. The BinaryUfuncInfo subclass also now allows specifying scalar support more precisely, and reference inputs and error inputs are generated based on this metadata to ensure it's correct. cc @kshitij12345 @pmeier @zou3519 @Chillee Pull Request resolved: #74280 Approved by: https://github.com/ngimel
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 25, 2026
…entwise binary operators This PR extends our OpInfo test architecture with "reference inputs," an optional expansion of typical sample inputs that allows for more thorough testing. Currently only the elementwise binary operations implement an extended set of reference inputs. This PR also cleans up some smaller OpInfo-related issues, including several bugs, and it identified pytorch#74279. A reference inputs function can be specified for an OpInfo by filling in its "reference_inputs_func" metadata. If this is done it's recommended that the reference inputs function first call the sample inputs function, then produce additional sample inputs. See `reference_inputs_elementwise_binary` for an example of this pattern. In addition to implementing reference inputs for the elementwise binary operations, this PR improves their consistency and simplifies how their metadata is represented. The great majority now use a generic sample input function, and those that want extensions start by calling the generic sample input function and then adding additional samples. This removes many older sample input functions. The BinaryUfuncInfo subclass also now allows specifying scalar support more precisely, and reference inputs and error inputs are generated based on this metadata to ensure it's correct. cc @kshitij12345 @pmeier @zou3519 @Chillee Pull Request resolved: pytorch#74280 Approved by: https://github.com/ngimel
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR extends our OpInfo test architecture with "reference inputs," an optional expansion of typical sample inputs that allows for more thorough testing. Currently only the elementwise binary operations implement an extended set of reference inputs. This PR also cleans up some smaller OpInfo-related issues, including several bugs, and it identified #74279.
A reference inputs function can be specified for an OpInfo by filling in its "reference_inputs_func" metadata. If this is done it's recommended that the reference inputs function first call the sample inputs function, then produce additional sample inputs. See
reference_inputs_elementwise_binaryfor an example of this pattern.In addition to implementing reference inputs for the elementwise binary operations, this PR improves their consistency and simplifies how their metadata is represented. The great majority now use a generic sample input function, and those that want extensions start by calling the generic sample input function and then adding additional samples. This removes many older sample input functions. The BinaryUfuncInfo subclass also now allows specifying scalar support more precisely, and reference inputs and error inputs are generated based on this metadata to ensure it's correct.
cc @kshitij12345 @pmeier @zou3519 @Chillee