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

x < min(x, y) #53836

Open
LilithHafner opened this issue Mar 24, 2024 · 11 comments
Open

x < min(x, y) #53836

LilithHafner opened this issue Mar 24, 2024 · 11 comments
Labels
docs This change adds or pertains to documentation

Comments

@LilithHafner
Copy link
Member

The minimum of x and anything should not be greater than x.

julia> x = typemax(UInt); y = Inf; x < min(x,y)
true

I found this issue when I got a trunc error on floor(T, min(typemax(T), x)) with T = UInt and x a float grater than typemax(UInt).

This is due to promotion changing the value of x to something not equal to x, and afaict it's not feasibe to fix this in the implementation, though it should be documented.

If someone has an idea for how to the behavior, though, that would be better.

@LilithHafner LilithHafner added the docs This change adds or pertains to documentation label Mar 24, 2024
@adienes
Copy link
Contributor

adienes commented Mar 25, 2024

I would consider this a "correctness bug"

julia> min(typemax(UInt), Inf)
1.8446744073709552e19

and min promises Return the minimum of the arguments which is not being done here

min(x, y) == (x, y)[argmin((x, y))] should always be true, imo

I understand that the types are promoted to enable comparison, but that shouldn't mean the return type changes

granted, that would harm the type-stability of min, as the return type of something like min(::Int, ::Float64) would depend on the magnitudes.. so maybe the "real" issue is the fact that convert(Float64, typemax(UInt64)) doesn't error? sure seems like it probably should...

@mikmoore
Copy link
Contributor

mikmoore commented Mar 25, 2024

Related to #52355. We currently provide that an Integer can always be converted to AbstractFloat even if this incurs roundoff (and Inf is returned for out-of range values such as in Float16(10^5)). As in discussed in #52355, we always resolve this using the default rounding mode (which is always RoundNearest for Base.IEEEFloat, since do not have proper ways to change the rounding mode for those numbers).

It's hard to say that we should (or even realistically could) do something better for min. This result follows very clearly from knowledge of promotion rules, but I agree that it's still a surprising enough result that maybe we could do something better to document it. There is a minor challenge in documenting this rare-in-practice pitfall in a way that increases clarity rather than diminishes it.

I guess this particular inspiring example floor(T, min(typemax(T), x)) should be rewritten as floor(typemax(T)) < x ? floor(typemax(T)) : floor(T, x) (the floors on the typemaxes are unnecessary for most types) so that it uses proper mixed-type comparisons. This implementation will throw on NaN, but one could tweak the condition to return floor(typemax(T)) instead.

@adienes
Copy link
Contributor

adienes commented Mar 25, 2024

for whatever it's worth, my opinion here and in #52355 is that they should definitely both throw InexactError
I believe this is what numpy does as well (well, actually they throw TypeError and disallow the comparison entirely)

documentation is probably a second-best solution. I am not so optimistic about the other suggestion with prevfloat and trying to catch a bunch of special cases etc.

@mikmoore
Copy link
Contributor

mikmoore commented Mar 25, 2024

I would accept an InexactError in #52355 because the docstring specifies it and it's still a MethodError on all released versions, although my personal-favorite outcome (performance considerations aside) would probably lean towards the prevfloat solution.

However, it would be very breaking to have promote throw on inexact conversions broadly. For the overwhelming majority of functions, the limited precision of the float is fine. I wouldn't want +(2^53+1, 1e5) to throw just because Float64(2^53+1) is inexact.

I don't see a way to fix min to be "right" in a type-stable way without introducing a bunch of InexactErrors (which I imagine would be a breaking change). If we ensure min(x,y) <= x && min(x,y) <= y for a type-stable min then we can't ensure min(x, y) == x || min(x,y) == y. It's only possible to satisfy both of these on post-promoted values.

@adienes
Copy link
Contributor

adienes commented Mar 25, 2024

for exactly the reasons you stated, I would have expected that min and max cannot be type-stable, in the same way that rand((1, 1.0)) cannot be type-stable. I understand of course this choice was made to improve performance, but nonetheless I think it's a "correctness bug"

@mbauman
Copy link
Member

mbauman commented Mar 25, 2024

It is quirky (and undocumented) that min and max promote for ::Real args in the first place. It's not a general or generic thing — the abstract fallbacks don't try to promote, even though they could! For example, there's no promotion in min(Date(2020), DateTime(2021)).

This dates way back to #212, when unions were indeed "nasty". Seems far less necessary these days.

@andrewjradcliffe
Copy link
Contributor

It's only possible to satisfy both of these on post-promoted values.

This highlights the core problem: the mixed-type comparison is itself ill-formed without explicit confirmation form the programmer. Promotion implicitly confirms that rounding error is acceptable; unless unless one wants to throw an error for all min(lhs::T, rhs::U) where T != U, it is the only reasonable choice.

@adienes
Copy link
Contributor

adienes commented Mar 28, 2024

it is the only reasonable choice.

surely it is at least "reasonable" (though up for debate whether "optimal") to do runtime checks if underflow has occurred and error if so, otherwise allow the comparison

I don't understand why this has only the doc label

maybe there is no path in sight to fixing it, but surely this is a bug!

@mikmoore
Copy link
Contributor

(Sidenote: this behavior also occurs in typemax(UInt) < minimum(Any[typemax(UInt),Inf]) so any change would need to happen there as well)

I don't see this as a bug. I see this as a simple consequence of Julia's ubiquitous promotion behavior. We still return the value of the minimum argument, just not necessarily with the original type. That conversion to floats can return an unequal value is a decision that was made long ago. But the existing implementation still satisfies arithmetic properties like !(x - min(x, y) < 0) (which I write this obtuse way to account for cases where the expression evaluates to NaN).

I think that even ignoring the status-quo motivator, there are compelling arguments for promoting min/max. One big pitfall of non-promoting min/max is that it's non-commutative (currently, it's only non-commutative on Real among pairs of NaNs, but those are at least isequal). For a few basic examples, what should the following calls return if they do not promote their arguments? What should their argument-reversed versions return?

min(0, 0e0)
min(0, -0e0)
min(Inf32, Inf64)
min(NaN32, NaN64) # note: it's already unspecified which of two NaNs are returned even when the same type

All of these examples except (0, -0e0) are already non-commutative in the === sense when the arguments are wrapped in 1-element vectors (which do a lexicographic min via isless).

These are not entirely meaningless distinctions. Operations like signbit(min(0, -0e0)) and inv(min(0, -0e0)) produce dramatically different results depending on which value is selected by min. I imagine most users expect commutativity out of min, at least for numbers, so sooner or later this would trip someone.

If some specific situation requires a min with specific semantics like min(x, y) === x || min(x, y) === y, ifelse or ?: are just a few keystrokes away. But I think that the promoting versions are better general-purpose implementations for Reals.

@mbauman
Copy link
Member

mbauman commented Mar 28, 2024

I see three somewhat-feasible options, from least- to most-breaking:

  • Document it — we currently don't say that min/max do promotion at all.
  • Add an error check to the heterogenous min(::Real, ::Real) method to ensure that the promoted values are == to the arguments.
  • Remove the promoting behavior from min(::Real, ::Real) method, returning to type instability.

The round-to-nearest behaviors of convert(Float64, x) are explicitly documented and breaking that would leave many Julia programs (and surely even building Julia itself) quite broken.


But this can also happen in other situations where it may not be feel as obviously buggy as the floor(T, min(typemax(T), x)) case does, including

julia> 1//10 < min(1//10, Inf)
true

julia> pi > max(pi, 0.0)
true

At the same time, we're quite lucky that pi rounds down; it's very easy to imagine doing a clamp(x, -pi, pi) to limit to some trigonometric domain. The analogous clamp(x, -1//10, 1//10) will give you floating point values outside $\big[\frac{-1}{10}, \frac{1}{10}\big]$... but at least clamp documents the promotion.

@andrewjradcliffe
Copy link
Contributor

andrewjradcliffe commented Mar 28, 2024

Document it — we currently don't say that min/max do promotion at all.

This seems to be the most robust choice. One might include in said documentation that for all things related to order, promotions are performed so as to return a type-stable result, at risk of loss of precision. If the programmer needs to perform heterogeneous order comparisons between floating point numbers, rational numbers and integers, then it should be their responsibility, as it is almost certain that the correct action (as @milkmore's and @mbauman's examples demonstrate) will depend on context.

In other words, homogeneous order comparison will have the behavior inherent to numbers outside a computer. For heterogeneous order comparisons, promotion is the default and if a programmer needs otherwise, they need to deal with it. This makes it straightforward to reason about ordering when dealing with two items of type T, much like Ord.

edit: credit to Matt's examples as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

No branches or pull requests

5 participants