Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Add min / max reduction #1012

Open
fzimmermann89 opened this issue Sep 29, 2024 · 6 comments · Fixed by #1057
Open

[Feature Request] Add min / max reduction #1012

fzimmermann89 opened this issue Sep 29, 2024 · 6 comments · Fixed by #1057
Assignees
Labels
enhancement New feature or request

Comments

@fzimmermann89
Copy link

Motivation

For supporting einops, tensordict would need a min/max reduction.

Solution

Similiar to mean, etc. the min and max should be implemented in https://github.com/pytorch/tensordict/blob/main/tensordict/base.py by calling _cast_reduction

I did not find any issue or explaination why this is missing..

@fzimmermann89 fzimmermann89 added the enhancement New feature or request label Sep 29, 2024
@vmoens
Copy link
Contributor

vmoens commented Oct 17, 2024

on it!

@vmoens
Copy link
Contributor

vmoens commented Oct 23, 2024

I'm working on this and I have a question for you:
what would be the most intuitive API:

  1. min(dim=dim) returns only the values. This breaks with torch.min(tensor, dim) which returns a namedtuple but feels "natural" otherwise
  2. min(dim=dim) returns a tensordict where the leaves are ("key0", "key1", "values") for the values and ("key0", "key1", "indices") for the indices
  3. min(dim=dim) returns a tensordict where the leaves are ("values", "key0", "key1") for the values and ("indices", "key0", "key1") for the indices
  4. min(dim=dim) returns a tensorclass with fields indices and values, where the leaves are ("key0", "key1") for each
  5. A namedtuple, as it is the case with pytorch

@fzimmermann89
Copy link
Author

If it were up to me, I would prefer .amin and .amax, both supporting multiple dimensions and only returning the value.

@vmoens
Copy link
Contributor

vmoens commented Oct 24, 2024

Cool I tried to do that. LMK if these changes make sense

@fzimmermann89
Copy link
Author

Is amin() here min().values? In torch, those two behave differently in the backward pass afaik

@vmoens
Copy link
Contributor

vmoens commented Oct 24, 2024

Oh good point, I should use amin directly then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants