Add clamp RFC #1961

Xaeroxe · 2017-03-27T05:31:45Z

RFC for feature discussed at https://internals.rust-lang.org/t/clamp-function-for-primitive-types/4999

Tracking issue: rust-lang/rust#44095
Implementation PR: rust-lang/rust#44097

killercup · 2017-03-27T08:07:15Z

kennytm · 2017-03-27T17:50:37Z

0000-clamp.md

+/// Returns the upper bound of the range if input is greater than the range, and the lower bound of
+/// the range if input is less than the range.  Otherwise this will return input.
+#[inline]
+pub fn clamp<T: Ord>(input: T, range: RangeInclusive<T>) -> T {


I'm not sure if we should tie the stability of this RFC with #1192 (rust-lang/rust#28237) here by using range: RangeInclusive<T> instead of min: T, max: T.

After messing around with the playground a bit and doing some thinking I actually agree with you. I much prefer the RangeInclusive syntax but the instability of it is too much of a drawback.

kornelski · 2017-03-28T10:04:09Z

My two concerns:

for floats semantics of NaN handling should allow implementation with the pair of maxsd & minsd.
I use clamp very often in conjunction with converting to an integer type, e.g. clamp(0,255) as u8. Should there be saturating_into() instead or in addition to clamp? In Rust the cast is a very expensive operation. SSE3 has FISTTP instruction that could perhaps be used to accelerate this?

bluetech · 2017-03-28T10:14:34Z

I think the RFC and docstring should explicitly describe what happens if max < min.

Xaeroxe · 2017-03-28T12:38:46Z

@pornel my first thought is that we ought to optimize the as operator rather than introducing new syntax to work around the performance problems it has. I would consider that suggestion to be outside the scope of this RFC. To be honest I don't really know what implications "for floats semantics of NaN handling should allow implementation with the pair of maxsd & minsd" would have on the implementation. If you could provide a code sample in Rust I might be able to better understand what you mean by that.

@bluetech agreed, I'll add something for that as soon as I can.

kornelski · 2017-03-28T14:39:30Z

@Xaeroxe please note that as doesn't clamp, so it's not a sufficient alternative.

I think use of SSE dictates that if any of the operands is NaN, then NaN is returned. http://www.felixcloutier.com/x86/MAXSD.html

kennytm · 2017-03-28T16:29:24Z

@pornel No,

If only one value is a NaN (SNaN or QNaN) for this instruction, the second source operand, either a NaN or a valid floating-point value, is written to the result.

i.e. MAXSD(NaN, 123.0) == 123.0.

The definition of MAXSD(a, b) is consistent with if a > b { a } else { b }.

ranma42 · 2017-03-28T16:47:27Z

text/0000-clamp.md

+        input
+    }
+}
+```


An alternative implementation which results in better code is:

pub fn clamp(input:f32, min: f32, max: f32) -> f32 { let mut x = input; if !(x < min) { x = min; } if !(x > max) { x = max; } x }

It conveniently preserves the source when it is NaN, as required in the edge cases listed below.

Thanks! Looks great!

... but it incorrectly returns NaN when max == NaN || min == NaN (EDIT: this is pseudocode... as crazy as it looks, the check for NaN would be max != max || min != min).
I am unsure if this is good or bad: while the infinity corresponding to no enforcement is straightforward, it is not obvious to me what is the desirable behaviour for NaN.

You know honestly that might be better behavior than what I proposed. It assumes a NaN is unintentional which they often are. If someone explicitly wants no bounds enforced they should provide infinity.

@ranma42 I think you mean if !(x >= min) etc.

ranma42 · 2017-03-28T16:50:17Z

Should we mention the expected behaviour for !(min < max) explicitly?

Xaeroxe · 2017-03-28T16:52:25Z

@ranma42 I think so yes. max and min may not be constant values so it's not that far out to say a user could accidentally provide a max < min

Xaeroxe · 2017-03-28T18:41:38Z

So I think the behavior for max < min should be explicitly defined but I don't really know what that behavior should be. My first thought is to just add a line at the top of the function:

assert!(max >= min);

I worry about
A: Is the performance impact of this undesirable
B: Is panicking really the best way to handle this state?

bluetech · 2017-03-28T18:52:10Z

If min and max are consts, then the assert will be elided by the optimizer.

If they are not statically known, and the function is marked inline, then if it is called inside of a loop and min and max are invariant then the optimizer will move the check out of the loop which makes it again practically free.

So IMO panic is the safe way to go.

ranma42 · 2017-03-28T18:54:42Z

@Xaeroxe that assert would also forbid passing NaN as max or min, which might be a good thing (and would simplify the edge cases table).

Xaeroxe · 2017-03-28T19:22:20Z

Thanks @bluetech ! I learned some cool stuff about the optimizer today.
@ranma42 I guess the question is do we want this function to propagate NaN or panic? Propagating NaN is what floating point operations typically do but I also am not sure if that's the best approach here. Propagating NaN doesn't cause the program to halt execution but it also can result in "error at a distance" type problems as well.

Xaeroxe · 2017-03-28T19:29:49Z

Personally I prefer panic, as more harsh consequences for invalid input are more likely to make the programmer make sure to account for the possibility of invalid input. Of course if anyone has counter points to that I'd love to hear them.

kennytm · 2017-03-28T19:40:13Z

@Xaeroxe I strongly suggest you revert the implementation back to what you had. @ranma42's implement is wrong (check with 4.clamp(3, 5)), and even when the typo is fixed it will interact badly when the input itself is NaN (check with NaN.clamp(3, 5)).

Xaeroxe · 2017-03-28T19:45:41Z

@kennytm Good catch. How about this?

pub fn clamp(input:f32, min: f32, max: f32) -> f32 {
    assert!(max >= min);
    let mut x = input;
    if x < min { x = min; }
    if x > max { x = max; }
    x
}

ranma42 · 2017-03-28T19:47:51Z

@kennytm sorry, you're right, I was misled by the documentation mentioning the first/second operand in Intel syntax vs the playground showing the GNU syntax. My bad.

ranma42 · 2017-03-28T19:49:15Z

@Xaeroxe I think assert!(min <= max) reads better, but yes, that's the original code by @kennytm, which is correct.

Xaeroxe · 2017-03-28T19:54:28Z

Changes made, requesting feedback.

Good work, I think we're getting closer to a final.

kennytm · 2017-03-28T20:24:01Z

For other languages/libraries having clamp or clip:

C++17's std::clamp (PR0025R0) implements exactly the same as this RFC before the last change (if x < lo { lo } else if hi < x { hi } else { x }). It asserts !(hi < lo) (NaNs can pass this assertion). Boost's boost::algorithm::clamp is equivalent but without the assert. (When not considering the assertion and optimizer, this algorithm is in fact produces the same output as the current RFC.)
Swift's clamped(to:) is range based, but when the self is a single point, is exactly the same as this RFC (if lo > x { lo } else if hi < x { hi } else { x }) (implementation.)

NumPy's clip() specializes for NaN, likely for performance of vectorized operation (implementation.), but otherwise the same as C++17.

if hi.is_nan() && lo.is_nan() {
    x
} else if hi.is_nan() {
    if x < lo { lo } else { x }
} else if lo.is_nan() {
    if x > hi { hi } else { x }
} else {
    if x < lo { lo } else if x > hi { hi } else { x }
}

GLib's CLAMP() is if x > hi { hi } else if x < lo { lo } else { x }.
Qt's qBound() is implemented as max(lo, min(hi, x)), which is expanded to (implementation.):
```
let y = if hi < x { hi } else { x }; 
if lo < y { y } else { lo }
```
GLSL's clamp is defined as min(max(x, lo), hi). Not sure how GLSL handles NaN. Also, Apple's Accelerate framework vector_clamp/simd::clamp is implemented using the same formula, with min and max corresponding to fmin and fmax. It differs from the current RFC in the handling of NaN.

Ruby's clamp is defined as

match x.partial_cmp(lo).unwrap() {
    Equal => x,
    Less => lo,
    _ => match x.partial_cmp(hi).unwrap() {
        Greater => hi,
        _ => x,
    }
}

Any NaN including the input will cause ArgumentError in Ruby, and there is an assertion for !(lo > hi).

Behaviors of different algorithms

lo	x	hi	C++17	Qt	GLSL	This RFC (ignoring assert)
3	6	5	hi (5)	hi (5)	hi (5)	hi (5)
3	5	5	x (5)	x (5)	x/hi (5)	x (5)
3	4	5	x (4)	x (4)	x (4)	x (4)
3	3	5	x (3)	lo (3)	x/lo (3)	x (3)
3	2	5	lo (3)	lo (3)	lo (3)	lo (3)
NaN	6	5	hi (5)	lo (NaN)	hi (5)	hi (5)
NaN	5	5	x (5)	lo (NaN)	x/hi (5)	x (5)
NaN	4	5	x (4)	lo (NaN)	x (4)	x (4)
3	NaN	5	x (NaN)	lo (3)	lo (3)	x (NaN)
3	4	NaN	x (4)	x (4)	x (4)	x (4)
3	3	NaN	x (3)	lo (3)	x/lo (3)	x (3)
3	2	NaN	lo (3)	lo (3)	lo (3)	lo (3)
NaN	NaN	5	x (NaN)	lo (NaN)	hi (5)	x (NaN)
NaN	4	NaN	x (4)	lo (NaN)	x (4)	x (4)
3	NaN	NaN	x (NaN)	lo (3)	lo (3)	x (NaN)
-0	+0	1	x (+0)	lo (-0)	x (+0)	x (+0)
+0	-0	1	x (-0)	lo (+0)	lo (+0)	x (-0)
-1	-0	+0	x (-0)	x (-0)	x (-0)	x (-0)
-1	+0	-0	x (+0)	x (+0)	hi (-0)	x (+0)
+0	-0	+0	x (-0)	lo (+0)	lo/hi (+0)	x (-0)
-0	+0	-0	x (+0)	lo (-0)	hi (-0)	x (+0)

aturon · 2017-08-09T15:08:54Z

The shepherds for this RFC have signaled readiness for full team review, so here we go!

@rfcbot fcp merge

rfcbot · 2017-08-09T15:08:54Z

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

aturon · 2017-08-26T02:36:30Z

Sorry for the delay -- hadn't realized this was waiting on @brson. Should go through now. Thanks, @Xaeroxe, for your long patience with this RFC!

rfcbot · 2017-08-26T02:36:31Z

🔔 This is now entering its final comment period, as per the review above. 🔔

Xaeroxe · 2017-08-26T06:12:01Z

Tracking issue: rust-lang/rust#44095
Implementation PR: rust-lang/rust#44097

rfcbot · 2017-09-05T02:38:21Z

The final comment period is now complete.

Xaeroxe · 2017-09-05T19:30:14Z

Implementation PR is ready, it can be merged as soon as this is merged I believe.

aturon · 2017-09-06T02:05:58Z

Merged! Thanks, @Xaeroxe, for sticking this one out.

Add clamp functions Implementation of clamp feature: Tracking issue: rust-lang#44095 RFC: rust-lang/rfcs#1961

Xaeroxe · 2017-09-09T23:48:22Z

Clamp was implemented then shortly reverted due to concerns about breaking downstream code. It's unlikely this RFC will be implemented. See this: rust-lang/rust#44095

jjpe · 2020-05-23T11:11:51Z

The rendered link on top is broken. I was looking this up to find out what clamp even is. Now I still don't know.

shepmaster · 2020-05-23T11:27:04Z

All accepted RFCs are visible on the website:

https://rust-lang.github.io/rfcs/1961-clamp.html

find out what clamp even is

https://doc.rust-lang.org/std/primitive.f32.html#method.clamp

Restrict a value to a certain interval unless it is NaN.

Returns max if self is greater than max, and min if self is less than min. Otherwise this returns self.

Not that this function returns NaN if the initial value was NaN as well.

scottmcm · 2021-02-11T17:20:52Z

Congrats, @Xaeroxe, on this finally getting to stable!

Xaeroxe · 2021-02-11T17:22:00Z

🎉 Thank you!! I'm very excited! 🎉

Xaeroxe added 2 commits March 26, 2017 23:30

Add clamp RFC

fc9dad4

Remove incorrect examples

3988071

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Mar 27, 2017

kennytm reviewed Mar 27, 2017

View reviewed changes

Xaeroxe mentioned this pull request Mar 27, 2017

Tracking issue for ..= inclusive ranges (RFC #1192) -- originally ... rust-lang/rust#28237

Closed

8 tasks

Move RFC into text and remove RangeInclusive use

ba9f88d

ranma42 reviewed Mar 28, 2017

View reviewed changes

Revise NAN behavior & optimize handling of floats.

1a6a796

Update 0000-clamp.md

0c1dbc5

Update RFC to panic if max is less than min.

b16f5bd

Fix float impl

2e31a67

rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Aug 9, 2017

rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Aug 26, 2017

This was referenced Aug 26, 2017

Tracking issue for clamp RFC rust-lang/rust#44095

Closed

Add clamp functions rust-lang/rust#44097

Merged

RFC 1961 introduces clamp functions

ac69333

aturon merged commit 9fad211 into rust-lang:master Sep 6, 2017

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 6, 2017

Rollup merge of rust-lang#44097 - Xaeroxe:clamp, r=burntsushi

98b1d8c

Add clamp functions Implementation of clamp feature: Tracking issue: rust-lang#44095 RFC: rust-lang/rfcs#1961

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 7, 2017

Rollup merge of rust-lang#44097 - Xaeroxe:clamp, r=burntsushi

44351ed

Add clamp functions Implementation of clamp feature: Tracking issue: rust-lang#44095 RFC: rust-lang/rfcs#1961

Xaeroxe deleted the Xaeroxe-clamp-rfc branch September 8, 2017 15:51

scottmcm mentioned this pull request Feb 26, 2019

Add clamp for ranges. Implements #44095 rust-lang/rust#58710

Merged

termoshtt mentioned this pull request Aug 17, 2019

Clip rust-ndarray/ndarray#470

Open

pyroxymat mentioned this pull request Mar 9, 2020

Implement min size constraint for splitter linebender/druid#613

Merged

KodrAus mentioned this pull request Nov 6, 2020

Stabilize clamp rust-lang/rust#77872

Merged

scottmcm mentioned this pull request Apr 12, 2021

Float min/max/clamp follow IEEE754-2008 (and not IEEE754-2019) WRT negative zeros rust-lang/rust#83984

Open

scottmcm mentioned this pull request Jun 21, 2021

Use RangeInclusive for fNN::lerp rust-lang/rust#86462

Closed

Xaeroxe mentioned this pull request Jan 1, 2022

Use of clamp in generics does not compose with f32 and f64, maybe relax restriction to PartialOrd? rust-lang/rust#85508

Closed

Add clamp RFC #1961

Add clamp RFC #1961

Conversation

Xaeroxe commented Mar 27, 2017 • edited

killercup commented Mar 27, 2017 • edited

kennytm Mar 27, 2017

Choose a reason for hiding this comment

Xaeroxe Mar 27, 2017

Choose a reason for hiding this comment

kornelski commented Mar 28, 2017 • edited

bluetech commented Mar 28, 2017

Xaeroxe commented Mar 28, 2017

kornelski commented Mar 28, 2017 • edited

kennytm commented Mar 28, 2017

ranma42 Mar 28, 2017

Choose a reason for hiding this comment

Xaeroxe Mar 28, 2017

Choose a reason for hiding this comment

ranma42 Mar 28, 2017 • edited

Choose a reason for hiding this comment

Xaeroxe Mar 28, 2017

Choose a reason for hiding this comment

kennytm Mar 28, 2017

Choose a reason for hiding this comment

ranma42 commented Mar 28, 2017 • edited

Xaeroxe commented Mar 28, 2017

Xaeroxe commented Mar 28, 2017

bluetech commented Mar 28, 2017

ranma42 commented Mar 28, 2017

Xaeroxe commented Mar 28, 2017

Xaeroxe commented Mar 28, 2017

kennytm commented Mar 28, 2017

Xaeroxe commented Mar 28, 2017

ranma42 commented Mar 28, 2017

ranma42 commented Mar 28, 2017

Xaeroxe commented Mar 28, 2017

kennytm commented Mar 28, 2017

aturon commented Aug 9, 2017

rfcbot commented Aug 9, 2017 • edited by aturon

aturon commented Aug 26, 2017

rfcbot commented Aug 26, 2017

Xaeroxe commented Aug 26, 2017 • edited

rfcbot commented Sep 5, 2017

Xaeroxe commented Sep 5, 2017

aturon commented Sep 6, 2017

Xaeroxe commented Sep 9, 2017

jjpe commented May 23, 2020

shepmaster commented May 23, 2020

scottmcm commented Feb 11, 2021 • edited

Xaeroxe commented Feb 11, 2021

Xaeroxe commented Mar 27, 2017 •

edited

killercup commented Mar 27, 2017 •

edited

kornelski commented Mar 28, 2017 •

edited

kornelski commented Mar 28, 2017 •

edited

ranma42 Mar 28, 2017 •

edited

ranma42 commented Mar 28, 2017 •

edited

rfcbot commented Aug 9, 2017 •

edited by aturon

Xaeroxe commented Aug 26, 2017 •

edited

scottmcm commented Feb 11, 2021 •

edited