Make π < π work as expected #27797
Merged
Make π < π work as expected #27797
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Methods are added for < and <= comparing Irrationals. (The corresponding > and >= are then handled by existing fallback methods.) Currently, expressions like `ℯ < π` result in promotion to Float64. This works most of the time. However, `π < π` does not result in promotion and throws an error. This commit introduces a method for comparing an Irrational to itself. This commit also introduces a test to make sure that incorrect results are not being returned due to different user-defined irrationals rounding to the same Float64.
simonbyrne
approved these changes
Jun 26, 2018
base/irrationals.jl
Outdated
| function <(x::AbstractIrrational, y::AbstractIrrational) | ||
| # If two different Irrationals round to the same Float64, then explicit | ||
| # < methods are needed for those inputs, as the below assertation will fail. | ||
| @assert Float64(x) != Float64(y) |
There was a problem hiding this comment.
This should probably throw some specific error.
There was a problem hiding this comment.
Probably a good idea. Something like
Float64(x) != Float64(y) || error("$x and $y have identical Float64 representations")
There was a problem hiding this comment.
Or a MethodError. The solution is to add a specific method, after all.
There was a problem hiding this comment.
I've added a commit, changing the code to throw a MethodError.
|
Did this work at one point? #8411 (comment) |
|
I thought it did, but I just tried various versions back to 0.4 and apparently not! |
In the (probably rare) case where a user defines two Irrationals that do not differ in their Float64 representation, comparisons will now throw a MethodError to inform the user that he needs to define the < method himself.
|
Cross-referencing the Discourse thread where this issue was raised: https://discourse.julialang.org/t/comparison-of-same-irrationals-by-throws-error/1342/ |
jrevels
pushed a commit
that referenced
this pull request
Jul 2, 2018
Methods are added for < and <= comparing Irrationals. (The corresponding > and >= are then handled by existing fallback methods.) Currently, expressions like `ℯ < π` result in promotion to Float64. This works most of the time. However, `π < π` does not result in promotion and throws an error. This commit introduces a method for comparing an Irrational to itself. This commit also introduces a test to make sure that incorrect results are not being returned due to different user-defined irrationals rounding to the same Float64.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Methods are added for < and <= comparing Irrationals. (The corresponding
methods for > and >= are then handled by existing fallback methods.)
Currently, expressions like
ℯ < πresult in promotion to Float64.This works most of the time. However,
π < πdoes not result inpromotion and throws an error. This commit introduces methods
for comparing an Irrational to itself.
This commit also introduces a test to make sure that incorrect results
are not being returned due to different user-defined irrationals
rounding to the same Float64.