Replace isfinite check in ranges with lo ≤ x ≤ hi#45646
Replace isfinite check in ranges with lo ≤ x ≤ hi#45646LilithHafner merged 10 commits intoJuliaLang:masterfrom
Conversation
|
Needs a test probably? |
|
@oscardssmith Review? |
|
This looks good, but ranges are very finicky, so this probably should get a pkgeval. |
base/range.jl
Outdated
| if !isfinite(x) | ||
| return false | ||
| elseif iszero(step(r)) | ||
| if iszero(step(r)) |
There was a problem hiding this comment.
| if iszero(step(r)) | |
| isnan(x) && return false | |
| if iszero(step(r)) |
The isfinite check implicitly covered NaNs. I think an explicit check for NaN is necessary if it's removed? Otherwise NaN in 1.0:2.0 would throw instead of returning false.
There was a problem hiding this comment.
Good catch! Unfortunately, that fix will not work because we don't have isnan for non-numeric ranges.
There was a problem hiding this comment.
I used first(r) <= x <= last(r) instead.
LilithHafner
changed the title
LilithHafner
changed the title
|
This should now also support empty ranges with undefined |
|
Is the set of assumptions we can make about an |
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
@nanosoldier |
|
Your package evaluation job has completed - no new issues were detected. A full report can be found here. |
|
PkgEval game back positive and I think this is ready to go if someone is willing to give it a final look over. |
|
looks good to me. |
KristofferC
added
backport 1.6
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com> (cherry picked from commit 5811825)
8 participants
This
isfinitecheck is a problem in at least two cases:a,a ∈ 1:a^2should holdAbstractRangestates "Supertype for ranges with elements of type T. UnitRange and other types are subtypes of this." (T need not be a number) whereasisfinite's docstring states "Test whether a number is finite." This check breaks range membership checks for non-numeric elements.The check does not maken = round(Integer, (x - first(r)) / step(r)) + 1much safer because it is possible to have a number that is too large to round to an Integer and still finite.The new check makes
n = round(Integer, (x - first(r)) / step(r)) + 1much safer, but it could still throw for ranges with length greater thantypemax(Int). Fixes #45747.