clamp function should check that "lo" < "hi"

[gasagna]

Hi,

playing around in math.jl I found that the clamp function does not check that "lo" is less than "hi", leading to unpredicted behaviour. For example clamp(5, 6, 4) returns 4, which I regard as a bug.

Cheers,

Davide

[staticfloat]

See #8007 for a possible solution

[staticfloat]

Tim Holy came up with a good compromise between speed and usability. The tension is between checking the bounds over and over again, (which is bad) and performing checking at all (which is good) so Tim suggested:

Build a ClampType which gets checked upon construction, then use it to clamp many different values without needing to check again?

The only bad thing about this is that it's not quite as simple as just calling clamp(). It would be great if the compiler could recognize that the work we're doing inside of clamp() could be hoisted out of loops, but that sounds pretty hard, so the next best thing we've got is the following:

clamper = Clamp(lo, hi) # <--- This can throw an exception
output_data = [clamper(x) for x in input_data]

[JeffBezanson]

If the clamp function, checks and all, is inlined, then the compiler will be able to hoist the check out of the loop (assuming the bounds are constant of course). Adding a ClampType is way too involved for such a simple function.

[staticfloat]

Then the approach in #8007, (quick link to the diff) as long as inlining is possible, could be performant?

[timholy]

If you really need it to be inlined and have trouble achieving that, see #8297. Fairly soon I hope to have another PR that will depend on that one. If no objections get raised in the meantime, that's the point I'll hit "merge."

[eschnett]

I don't like the idea of an "undefined" result for such a simple function as clamp. Would it really be too difficult to modify @inbounds, or to introduce a new function @inrange?

The latter (@inrange) would also be useful for sqrt, asin, log, etc., removing the need for checking for input that would lead to a nan or an exception.

[StefanKarpinski]

What are you proposing that the effect of @inrange would be?

[JeffBezanson]

I feel there is an obvious implementation of clamp in terms of comparison
and ?. That is what you would write if the function didn't exist. So, I
think clamp can be defined in terms of that implementation. No checks, no
undefined cases.

[eschnett]

Jeff

There is no single obvious implementation -- there are two obvious implementations, depending on whether you perform the "min" or the "max" operation first. If you change the order, the result will differ iff minval>maxval.

Actually, the OpenCL standard defines clamp as fmin(fmax(x, minval), maxval). Maybe we should use this as the obvious implementation.

[eschnett]

Stefan

@inrange would omit the range check that some find offensive since perceived as too expensive in some occasions. The use case would be the same as for @inbounds -- usually not necessary if llvm is clever enough to hoist the check out of the loop, but sometimes still improving performance.

[JeffBezanson]

Yes, +1 for the opengl approach. Obvious might not be the right word, but
opengl did the right thing by establishing a definition like that.

[staticfloat]

We basically have the same functionality as OpenCL right now, so should we just close this as wontfix?

[vtjnash]

agreed

[eschnett]

There could be a checked_clamp, folded into general checked arithmetic that could be made available as a package.