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.

Sign up for GitHub

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

Add ishalfodd function #59

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Add ishalfodd function #59

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3798c03
add `ishalfoddinteger` function
hyrodium Nov 26, 2023
77e1c50
add tests for `ishalfoddinteger`
hyrodium Nov 26, 2023
2c22cfd
add support for old versions of Julia
hyrodium Nov 26, 2023
cf96fc4
add support for old versions of Julia
hyrodium Nov 26, 2023
2d8273b
add argument type specification `::Number`
hyrodium Dec 5, 2023
21d121a
remove `ishalfoddinteger(::AbstractIrrational)`
hyrodium Dec 5, 2023
b0bc2e5
rename `ishalfoddinteger` to `ishalfodd`
hyrodium Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/HalfIntegers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
# Functions
half,
ishalfinteger,
ishalfoddinteger,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename this function to ishalfodd, like isinteger and isodd?

Copy link
Owner

Choose a reason for hiding this comment

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

I don’t think it needs to be renamed (although it is quite long). To me, ishalfoddinteger seems clearer than ishalfodd, but I’m fine with either name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review! I named the function referring to the word "half-odd-integers" in the Wikipedia article.
But I now think consistency with Base.odd and Base.isinteger has priority, so I will update the name. (I think ishalfodd is still clear enough.)

Copy link
Owner

Choose a reason for hiding this comment

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

I don’t understand this. To me, ishalfodd seems consistent with Base.isodd and ishalfoddinteger seems consistent with Base.isinteger. Why do you think that ishalfoddinteger is more consistent with both of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for confusing. I was just trying to clarify the pros and cons of these function names. The reason of ishalfoddinteger was just borrowing from the wikipedia article. I now changed the name to ishalfodd which is consistent with Base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#59 (comment) was written before the commit b0bc2e5.

onehalf,
twice

Expand Down Expand Up @@ -561,6 +562,38 @@
ishalfinteger(::AbstractIrrational) = false
ishalfinteger(::Missing) = missing

"""
ishalfoddinteger(x)

Test whether `x` is numerically equal to some half-odd-integer.

# Examples

```jldoctest
julia> ishalfoddinteger(3.5)
true

julia> ishalfoddinteger(2)
false

julia> ishalfoddinteger(1//3)
false
```
"""
ishalfoddinteger(x) = _isodd(twice(x))
ishalfoddinteger(x::Rational) = denominator(x) == 2
ishalfoddinteger(::Integer) = false
ishalfoddinteger(::AbstractIrrational) = false
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is not necessarily correct, because nothing prevents a user from defining an AbstractIrrational subtype that is equal to a rational number. The AbstractIrrational docstring even mentions this possibility:

If a subtype is used to represent values that may occasionally be rational (e.g. a square-root type that represents √n for integers n will give a rational result when n is a perfect square), then it should also implement isinteger, iszero, isone, and == with Real values (since all of these default to false for AbstractIrrational types), as well as defining hash to equal that of the corresponding Rational.

Since we cannot add ishalfoddinteger to that list, maybe we should let it fall back to the generic _isodd(twice(x))? Or should we just add a note to the ishalfoddinteger docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. I added the method referring to the Base.isinteger method.

https://github.com/JuliaLang/julia/blob/9c6fec13ed3a106a4c1e30d07d55eaa3701b5a59/base/irrationals.jl#L143

I now think we can just drop the method definition, and ishalfinteger(x::Number) can handle it.

ishalfoddinteger(::Missing) = missing

if VERSION ≥ v"1.7"
sostock marked this conversation as resolved.
Show resolved Hide resolved
_isodd(x) = isodd(x)
else
_isodd(x::Real) = isodd(x)

Check warning on line 592 in src/HalfIntegers.jl

View check run for this annotation

Codecov / codecov/patch

src/HalfIntegers.jl#L592 

Added line #L592 was not covered by tests
_isodd(n::Number) = isreal(n) && _isodd(real(n))
_isodd(x::AbstractFloat) = isinteger(x) && abs(x) ≤ maxintfloat(x) && _isodd(Integer(x))
end

@static if VERSION ≥ v"1.7.0-DEV.263"
Base.range_start_stop_length(start::T, stop::T, len::Integer) where T<:HalfInteger =
Base._linspace(float(T), start, stop, len)
Expand Down
1 change: 1 addition & 0 deletions test/customtypes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Base.convert(::Type{One}, x::Int) = x == 1 ? One() : error("can't convert $x to
@testset "Properties" begin
@test isfinite(MyHalfInt(2))
@test ishalfinteger(MyHalfInt(3/2))
@test ishalfoddinteger(MyHalfInt(3/2))
@test isinteger(MyHalfInt(3))
@test !isinteger(MyHalfInt(3/2))
@test iszero(MyHalfInt(0))
Expand Down
19 changes: 19 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,24 @@ end
@test !ishalfinteger(im)
end

@testset "ishalfoddinteger" begin
for T in (halfinttypes..., halfuinttypes..., :BigHalfInt)
@eval @test !ishalfoddinteger($T(3))
@eval @test ishalfoddinteger($T(5/2))
end
@test !ishalfoddinteger(5)
@test !ishalfoddinteger(big(-5))
@test ishalfoddinteger(5//2)
@test ishalfoddinteger(-3//2)
@test !ishalfoddinteger(4//3)
@test !ishalfoddinteger(2.0)
@test ishalfoddinteger(-7.5)
@test !ishalfoddinteger(2.3)
@test !ishalfoddinteger(π)
@test !ishalfoddinteger(ℯ)
@test !ishalfoddinteger(im)
end

@testset "isinteger" begin
for T in (halfinttypes..., :BigHalfInt)
@eval @test isinteger($T(3))
Expand Down Expand Up @@ -1690,6 +1708,7 @@ end
@test onehalf(missing) === onehalf(Missing) === missing

@test ishalfinteger(missing) === missing
@test ishalfoddinteger(missing) === missing
end

include("ranges.jl")
Expand Down
Loading