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

Reconsider min implementation as negative of max #904

Open
ricardoV94 opened this issue Jul 8, 2024 · 0 comments
Open

Reconsider min implementation as negative of max #904

ricardoV94 opened this issue Jul 8, 2024 · 0 comments

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Jul 8, 2024

Description

For historical reasons pt.min returns pt.neg(pt.max(pt.neg(x))). This seems to have been the case mostly to avoid having to define Min Op and its gradient. There is a later "uncanonicalize" phase that converts those expressions to min, suggesting we prefer them, but don't put it in place because of the lack of gradient.

We should reassess this as is adds some unwelcome complexity. The L_op implementation (cleaned up in #901) works directly for min. R_op, on the other hand uses Argmax (not sure why this is needed in the forward but not backward pass, CC @aseyboldt), so a similar Min.R_op may need to use Argmin. Similar to Min that's currently implemented as Argmax of negative of x, which is probably fine? We could also consider a direct Argmin but that is not as ubiquitous and hence less annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant