Add namedtuple return for min, median, mode, kthvalue, add test for namedtuple return API#16186
Add namedtuple return for min, median, mode, kthvalue, add test for namedtuple return API#16186zasdfgbnm wants to merge 65 commits intopytorch:masterfrom
Conversation
|
|
||
| path = os.path.dirname(os.path.realpath(__file__)) | ||
| aten_native_yaml = os.path.join(path, '../aten/src/ATen/native/native_functions.yaml') | ||
| whitelist = [ |
There was a problem hiding this comment.
Don't we intend to give all functions named outputs in short order? In this case, I don't feel like this test case is giving us much utility, except perhaps reminding us to carefully audit choice of named outputs. What do you think?
There was a problem hiding this comment.
@ezyang Yes, I added this test simply to remind people be careful on these names, because this might be a point people could easily forget. I'm OK with removing it if this won't be a problem.
|
@ezyang There is no strong reason for disabling the name of fractional max pool, just because this name was given before the nametuple return merged. So I'm planning to disable it for now and add back in later PRs. Do you think I should revert the changes on fractional max pool? Edit: I think at least we should rename the return name of fractional max pool from (output, indices) to (values, indices), which matches with the output of |
|
I think I'm OK to codify the outputs of FractionalMaxPool as You need to rebase this PR, is that right? |
|
@ezyang This should be ready for review now after merging with master. Is it OK that I remove the return name of fractional max pooling in this PR, and add them back in following PRs together with other max pooling operators (including 1d and 2d fractional max pooling)? |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Ugh, this needs a merge with master now :( |
|
I attempted a merge, if you could look it over that would be appreciated. |
|
@ezyang Did you just resolve a merge conflict? I read through this diff, it looks good to me. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…amedtuple return API (#16186) Summary: This partially fixes #394 and depend on #15429. I suggest to review this only after #15429 get landed, otherwise the diff might be large to review. The test only allows explicitly whitelisted operators to have named return. Differential Revision: D14070735 Pulled By: ezyang fbshipit-source-id: ace2a672998b4e4a8094f52cbda5aa1cea6e3b42
| >>> torch.kthvalue(x,2,0,True) | ||
| (tensor([[ 4., 5., 6.]]), tensor([[ 1, 1, 1]])) | ||
| >>> torch.kthvalue(x, 2, 0, True) | ||
| torch.return_types.kthvalue(values=tensor([[4., 5., 6.]]), indices=tensor([[1, 1, 1]])) |
There was a problem hiding this comment.
Can we do something to avoid printing this really long type? We can pretend that all of them are a ResultTuple or sth like that
There was a problem hiding this comment.
We are customizing the printing at #17136
Summary: More ops for #394. ~~Also need to rebase after landing #16186, because we need to update the whitelist of the new unit test added in #16186.~~ cc: ezyang Pull Request resolved: #17093 Differential Revision: D14620068 Pulled By: ezyang fbshipit-source-id: deec5ffc9bf7624e0350c85392ee59789bad4237
This partially fixes #394 and depend on #15429. I suggest to review this only after #15429 get landed, otherwise the diff might be large to review.
The test only allows explicitly whitelisted operators to have named return.
cc: @ezyang