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

Replace isfinite check in ranges with lo ≤ x ≤ hi #45646

Merged
merged 10 commits into from
Oct 3, 2022

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Jun 11, 2022

This isfinite check is a problem in at least two cases:

  • Working with infinite ranges. If I'm working in nonstandard arithmetic with nonstandard element a, a ∈ 1:a^2 should hold
  • Working with ranges over non-numbers The docstring of AbstractRange states "Supertype for ranges with elements of type T. UnitRange and other types are subtypes of this." (T need not be a number) whereas isfinite's docstring states "Test whether a number is finite." This check breaks range membership checks for non-numeric elements.

The check does not make n = round(Integer, (x - first(r)) / step(r)) + 1 much 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)) + 1 much safer, but it could still throw for ranges with length greater than typemax(Int). Fixes #45747.

@giordano
Copy link
Contributor

Needs a test probably?

@N5N3 N5N3 added the ranges Everything AbstractRange label Jun 11, 2022
@ViralBShah ViralBShah changed the title Remove isfinite check Remove isfinite check in ranges Jun 18, 2022
@ViralBShah
Copy link
Member

@oscardssmith Review?

@oscardssmith
Copy link
Member

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Unfortunately, that fix will not work because we don't have isnan for non-numeric ranges.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used first(r) <= x <= last(r) instead.

@LilithHafner
Copy link
Member Author

LilithHafner commented Jun 19, 2022

Here's a related issue inspired by @vyu's comment: 1e40 ∈ 0:1.0 throws on conversion to Integer.

@LilithHafner LilithHafner changed the title Remove isfinite check in ranges Replace isfinite check in ranges with first(r) ≤ x ≤ last(r) Jun 19, 2022
@LilithHafner LilithHafner changed the title Replace isfinite check in ranges with first(r) ≤ x ≤ last(r) Replace isfinite check in ranges with lo ≤ x ≤ hi Jul 6, 2022
@LilithHafner
Copy link
Member Author

This should now also support empty ranges with undefined first.

base/range.jl Show resolved Hide resolved
@LilithHafner
Copy link
Member Author

Is the set of assumptions we can make about an AbstractRange documented somewhere?
For example, can we assume first(r) + (length(r)-1) * step(r) ≈ last(r)?

@LilithHafner
Copy link
Member Author

This looks good, but ranges are very finicky, so this probably should get a pkgeval.

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@LilithHafner
Copy link
Member Author

@nanosoldier runtests(["BasisFunctions", "HorseML", "OutlierDetectionData", "SimpleChains", "StableTrees", "TensorBoardLogger"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

@LilithHafner
Copy link
Member Author

PkgEval game back positive and I think this is ready to go if someone is willing to give it a final look over.

@oscardssmith
Copy link
Member

looks good to me.

@LilithHafner LilithHafner merged commit 5811825 into JuliaLang:master Oct 3, 2022
@LilithHafner LilithHafner deleted the patch-2 branch October 3, 2022 00:02
@KristofferC KristofferC added backport 1.6 Change should be backported to release-1.6 backport 1.8 Change should be backported to release-1.8 labels Oct 3, 2022
KristofferC pushed a commit that referenced this pull request Oct 27, 2022
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
@KristofferC KristofferC mentioned this pull request Oct 27, 2022
37 tasks
KristofferC pushed a commit that referenced this pull request Oct 28, 2022
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Nov 8, 2022
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
KristofferC pushed a commit that referenced this pull request Oct 10, 2023
Co-authored-by: Viral B. Shah <[email protected]>
(cherry picked from commit 5811825)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6 ranges Everything AbstractRange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

∈ throws for Float64 with large value with small range
8 participants