-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
src/HalfIntegers.jl
Outdated
@@ -23,6 +23,7 @@ export | |||
# Functions | |||
half, | |||
ishalfinteger, | |||
ishalfoddinteger, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
src/HalfIntegers.jl
Outdated
ishalfoddinteger(x) = _isodd(twice(x)) | ||
ishalfoddinteger(x::Rational) = denominator(x) == 2 | ||
ishalfoddinteger(::Integer) = false | ||
ishalfoddinteger(::AbstractIrrational) = false |
There was a problem hiding this comment.
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 integersn
will give a rational result whenn
is a perfect square), then it should also implementisinteger
,iszero
,isone
, and==
with Real values (since all of these default tofalse
forAbstractIrrational
types), as well as defininghash
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?
There was a problem hiding this comment.
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.
I now think we can just drop the method definition, and ishalfinteger(x::Number)
can handle it.
Gentle bump 😄 |
Sorry for the long wait! I will make a release with this soon. |
Thank you for the merge! |
This PR adds a new function
ishalfoddinteger
.https://en.wikipedia.org/wiki/Half-integer