Skip to content

Revert "Adding argmax index as second return value to max/min calls without specified dimension" - #523

Merged
dominikgrewe merged 1 commit into
masterfrom
revert-489-master
Feb 2, 2016
Merged

Revert "Adding argmax index as second return value to max/min calls without specified dimension"#523
dominikgrewe merged 1 commit into
masterfrom
revert-489-master

Conversation

@dominikgrewe

Copy link
Copy Markdown
Member

Reverts #489

@dominikgrewe

Copy link
Copy Markdown
Member Author

We noticed this is breaking code when the output of max or min is passed to another function. Previously one argument would be passed, now two are passed, causing the function to behave differently.

dominikgrewe added a commit that referenced this pull request Feb 2, 2016
Revert "Adding argmax index as second return value to max/min calls without specified dimension"
@dominikgrewe
dominikgrewe merged commit c86f2a5 into master Feb 2, 2016
soumith added a commit to torch/nn that referenced this pull request Feb 2, 2016
@dominikgrewe

Copy link
Copy Markdown
Member Author

One possible solution is this: Only return the argmax if an index is passed. An index of -1 means that the max should be computed over the entire tensor and the argmax is returned as in the reverted PR. If no index is passed, the max is also computed over the entire tensor, but the argmax is not returned (just like before).

t = torch.Tensor{{1, 2, 3}, {4, 5, 6}}
= t:max()
6

= t:max(1)
 4  5  6
[torch.DoubleTensor of size 1x3]

 2  2  2
[torch.LongTensor of size 1x3]

= t:max(-1)
6

 2  3
[torch.LongTensor of size 2]

Any thoughts?

@soumith
soumith deleted the revert-489-master branch February 2, 2016 18:12
@soumith

soumith commented Feb 2, 2016

Copy link
Copy Markdown
Member

@dominikgrewe isn't that already the case on trunk?

@soumith

soumith commented Feb 2, 2016

Copy link
Copy Markdown
Member

the only difference with the version in trunk, then is that instead of returning just one of the max indices, it returns all max indices.

@dominikgrewe

Copy link
Copy Markdown
Member Author

Currently, if you don't specify a dimension, no index is returned.

@soumith

soumith commented Feb 2, 2016

Copy link
Copy Markdown
Member

oh, i see that you want to introduce a -1, that seems okay to me. and it wouldn't break tons of code.
When a PR is prepared for this, can you also make sure the cutorch PR is prepared in parallel.

@dominikgrewe

Copy link
Copy Markdown
Member Author

Sure, will do.

tzanko-matev pushed a commit to logic-star-ai-tests/pytorch__pytorch that referenced this pull request Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants