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

Add ishalfodd function #59

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Conversation

hyrodium
Copy link
Contributor

This PR adds a new function ishalfoddinteger.

julia> ishalfoddinteger(3.5)
true
julia> ishalfoddinteger(2)
false
julia> ishalfoddinteger(1//3)
false

Note that halving an integer does not always produce a half-integer; this is only true for odd integers. For this reason, half-integers are also sometimes called half-odd-integers.

https://en.wikipedia.org/wiki/Half-integer

@@ -23,6 +23,7 @@ export
# 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.

Copy link

codecov bot commented Nov 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d4dc006) 98.81% compared to head (b0bc2e5) 98.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   98.81%   98.85%   +0.03%     
==========================================
  Files           1        1              
  Lines         253      261       +8     
==========================================
+ Hits          250      258       +8     
  Misses          3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

@hyrodium hyrodium changed the title Add ishalfoddinteger function Add ishalfodd function Dec 5, 2023
@hyrodium
Copy link
Contributor Author

Gentle bump 😄

@sostock sostock merged commit 4e51170 into sostock:master Mar 11, 2024
51 checks passed
@sostock
Copy link
Owner

sostock commented Mar 11, 2024

Sorry for the long wait! I will make a release with this soon.

@hyrodium
Copy link
Contributor Author

Thank you for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants