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

[sdlib] Add clamp to math module #3039

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from

Conversation

bgreni
Copy link
Contributor

@bgreni bgreni commented Jun 13, 2024

  • Add clamp to the math module
  • Replace private _clip function in List with clamp

Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

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

I'm not in favor or against, just adding info here. In the Python stdlib, it's been discussed to add such a function in this mailing list the TLDR of the mailing list is that there are many different implementations of clamp in the wild and they slightly differ on the way NaN are handled. In the end it was not added to the stdlib due to the lack of concensus, mostly about NaN.

Maybe we should add information about how this new function handles NaN in the docstring? Adding a test with NaN would be good too as we should not change the way we handle them as time goes by.

@soraros
Copy link
Contributor

soraros commented Jun 19, 2024

We should also document how the max > min case is handled. There is another way out: define clamp as composition of min and max (both stablehlo and XLA took this approach), so it's not implementation detail anymore.

In other words, we can take the syntactic trait idea from #2924, and make clamp a generic function over types with HasMax and HasMin.

@bgreni
Copy link
Contributor Author

bgreni commented Jun 19, 2024

Maybe we should add information about how this new function handles NaN in the docstring?

Since a clamp method already exists on SIMD, we'll have to just go with whatever behaviour it has?

We should also document how the max > min case is handled.

Out of curiosity I checked the std::clamp documentation and of course it's undefined behaviour...
Personally I wouldn't mind simply swapping lower and upper in that case?

Signed-off-by: Brian Grenier <[email protected]>
@laszlokindrat
Copy link
Contributor

Thanks for the patch! Is there a direct motivation for this change? As pointed out, Python's math module doesn't have a clamp method, so we don't need this for compatibility. There is also a SIMD.clamp method already, so I'm not sure we should bring this in yet.

@bgreni
Copy link
Contributor Author

bgreni commented Jun 28, 2024

@laszlokindrat I noticed we had a private function for this in List and figured we might as well add it to the math module as a public function where it's properly tested and accounted for, and while it isn't included in Python, the C++ people might appreciate it's inclusion in this form as a free function like std::clamp.

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.

None yet

4 participants