Skip to content

Conversation

peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented Jan 8, 2021

Stack from ghstack:

This allows different sample inputs to have different behavior for the same
operator. For example, div(..., rounding_mode='true') will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

Differential Revision: D25950011

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 8, 2021

💊 CI failures summary and remediations

As of commit 7a5be92 (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.

…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
@peterbell10 peterbell10 requested a review from mruberry January 8, 2021 17:04
…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
peterbell10 added a commit to peterbell10/pytorch that referenced this pull request Jan 9, 2021
This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

ghstack-source-id: 7fca427
Pull Request resolved: pytorch#50279
@mruberry
Copy link
Collaborator

Implementing dunder repr for SampleInputs and more metadata for whether the function implements safe casting sounds good. Do we really want to get rid of the metadata for whether the operator promotes integers to float, though? That seems useful.

I understand that with the changes to torch.divide(...) we have a function that conditionally promotes integers to floating point. Is a better way to model this function that torch.divide(..., mode=X) is a different function from torch.divide(..., mode=Y), though?

…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
@peterbell10
Copy link
Collaborator Author

Note that either way, promote_integers_to_float is not general enough to make these type of tests work for all operators. For example, torch.fft.fft promotes integers to complex not float and torch.count_nonzero returns long from bool inputs. The approach I just pushed of calling c10::canCast on the other hand is completely general.

…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
@mruberry
Copy link
Collaborator

Note that either way, promote_integers_to_float is not general enough to make these type of tests work for all operators. For example, torch.fft.fft promotes integers to complex not float and torch.count_nonzero returns long from bool inputs. The approach I just pushed of calling c10::canCast on the other hand is completely general.

Cool; I'll take a look. Another option would be to provide sugars for common cases (like int to float promotion), but also allowing OpInfos to override with an arbitrary mapping of inputs -> expected result dtype.

@mruberry
Copy link
Collaborator

From our conversation and looking at the latest I'm convinced. It is a bit of a pain that the op has to be run to determine the output dtype, but c'est la vie. I think, however, we can use torch.can_cast and that should fix the failing builds.

…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
@peterbell10
Copy link
Collaborator Author

@mruberry PTAL, torch.can_cast did indeed fix CI.

…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

[ghstack-poisoned]
@mruberry mruberry self-requested a review January 19, 2021 07:33
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Cool!

@mruberry
Copy link
Collaborator

Hey @peterbell10, unfortunately the internal tools can't figure out to merge. I know these rebases are out of control, but would you mind rebasing this so I can reimport it?

…tead"

This allows different sample inputs to have different behavior for the same
operator. For example, `div(..., rounding_mode='true')` will promote but other
rounding modes don't. The current boolean flag is too restrictive to allow this.

Differential Revision: [D25950011](https://our.internmc.facebook.com/intern/diff/D25950011)

[ghstack-poisoned]
@peterbell10
Copy link
Collaborator Author

@mruberry rebased.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in 0436ea1.

@facebook-github-bot facebook-github-bot deleted the gh/peterbell10/35/head branch January 26, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants