-
Notifications
You must be signed in to change notification settings - Fork 30
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 fmpz_is_square #172
add fmpz_is_square #172
Conversation
src/flint/types/fmpz.pyx
Outdated
def is_perfect_power(self): | ||
r""" | ||
Return True if this integer is of the form `r^k`, False otherwise. | ||
|
||
>>> fmpz(81).is_perfect_power() | ||
True | ||
>>> fmpz(1234).is_perfect_power() | ||
False | ||
""" | ||
cdef int k | ||
cdef fmpz v = fmpz() | ||
k = fmpz_is_perfect_power(v.val, self.val) | ||
return k != 0 |
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.
We can have both methods but more useful than is_perfect_power
would be a method that returns the power like SymPy's perfect_power
function:
In [1]: perfect_power(81)
Out[1]: (3, 4)
In [2]: perfect_power(80)
Out[2]: False
I'm not sure what is best to return if it is not a perfect power. Returning False is perhaps a bit strange...
Alternatives:
- Raise an exception.
- Return clearly invalid number like
(0, -1)
. - Return a tuple with a boolean like
(3, 4, True)
or(0, -1, False)
. - Return None.
Maybe returning False
is good as anything else...
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.
Or perhaps it can just be:
>>> perfect_power(80)
(80, 1)
So perfect_power(n)
always returns (r, k)
such that n = r^k
where k
is either maximal or 1
if no maximal k
exists:
>>> perfect_power(1)
(1, 1)
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.
is_perfect_power
was there before this PR, but when I saw it, I also thought it should best return (r,k)
since that data is provided by the C function (in that case I would lean towards None
if not a perfect power), so I'd be happy to change that.
Are you also suggesting it be renamed to perfect_power
? That could make sense since one might expect a method called is_something
to return a boolean. On the other hand, it seems the currently prevailing convention is to mirror the C library names, so maybe it should stay is_perfect_power
?
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 I missed the fact that the function was already there. I guess it is better not to change it then.
We could add a separate .perfect_power()
method though to return (r, k)
. The convention it seems in other places is that a perfect power should have r,k>=2
although I guess that the .perfect_power()
method should be consistent with is_perfect_power()
.
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 that would be good. The issue is though that (r,k)
returned by fmpz_is_perfect_power
is not guaranteed to have maximal k
; the right way to proceed would be to implement that feature in flint's fmpz_is_perfect_power
and then use it in python-flint to implement a method .perfect_power()
as you are suggesting.
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, okay. I suppose we could just call fmpz_is_perfect_power
in a loop until we've reduced down to a number that is not a perfect power.
Maybe returning a result with non-maximal k
is useful. I would be more inclined to reduce as far as possible (i.e. find maximal k
). That is what Sage's perfect_power
apparently does:
https://doc.sagemath.org/html/en/reference/rings_standard/sage/rings/integer.html#sage.rings.integer.Integer.perfect_power
This seems like the sort of situation where what is best in python-flint might be a bit different from what Flint itself does. I'm not sure what the performance implications would be though of calling fmpz_is_perfect_power
in a loop (with successively smaller arguments) compared to just calling it once.
Can you add tests in Doctests are good for some simple things but generally it is better to have comprehensive tests including for edge cases outside of the doctests. We want full test coverage from the test suite and then doctests are good for showing tested examples in the documentation rather than actually serving as the test suite. I'm not sure about these ones: In [6]: flint.fmpz(-1).is_perfect_power()
Out[6]: True
In [7]: flint.fmpz(0).is_perfect_power()
Out[7]: True
In [8]: flint.fmpz(1).is_perfect_power()
Out[8]: True Do we actually want that? SymPy's perfect power rejects -1, 0 and 1. Wikipedia suggests that 0 and 1 are sometimes considered perfect powers but no mention of -1. It seems that flint's function like SymPy's accepts negative odd powers though so I guess it makes sense to have -1 if we also have e.g. -8. |
Okay, looks good. We can merge this once tests are complete unless you want to add a |
Thanks, I prefer to merge this now. |
Great. Thanks! |
No description provided.