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

is_power and is_prime_power_with_data are inconsistent #2352

Closed
lgoettgens opened this issue May 6, 2023 · 6 comments · Fixed by Nemocas/Nemo.jl#1484
Closed

is_power and is_prime_power_with_data are inconsistent #2352

lgoettgens opened this issue May 6, 2023 · 6 comments · Fixed by Nemocas/Nemo.jl#1484
Labels
bug Something isn't working

Comments

@lgoettgens
Copy link
Member

lgoettgens commented May 6, 2023

I would expect both methods to return basically the same (up to the boolean flag). However, the order of base and exponent is different.

is_power(::IntegerUnion) gives first the exponent and then the base.

help?> is_power
  is_power(a::ZZRingElem) -> Int, ZZRingElem
  is_power(a::Integer) -> Int, Integer

  Returns e, r such that a = r^e with e maximal. Note: 1 = 1^0.

On the other hand, is_prime_power_with_data(::IntegerUnion) gives after the boolean flag first the base and then the exponent.

help?> is_prime_power_with_data
search: is_prime_power_with_data

  is_prime_power_with_data(q::IntegerUnion) -> Bool, ZZRingElem, Int

  Returns a flag indicating whether q is a prime power and integers p, e such that q = p^e. If q is a prime power, than p
  is a prime.

Finally, for is_power(::QQFieldElem), the docstring specifies no behavior at all (although it again is exponent, base).

  is_power(a::QQFieldElem) -> Int, QQFieldElem
  is_power(a::Rational) -> Int, Rational

  Writes a = r^e with e maximal. Note: 1 = 1^0.

I would propose adding the order into the docstring of is_power(::QQFieldElem).
Furthermore, I think it would be less confusing to make these two functions consistent in their return semantics.

@lgoettgens lgoettgens added the bug Something isn't working label May 6, 2023
@fieker
Copy link
Contributor

fieker commented May 8, 2023 via email

@lgoettgens
Copy link
Member Author

Well spotted - there is actually a 3rd point to consider... in Oscar/example/PerfectPowers.jl is a far superior implementation for large integers...

is_power_bernstein(::ZZRingElem) from this file only returns the exponent.

Do you happen to have any idea what the order is for is_power for ideals and/or polynomials? I assume the is_prime_power_with_data is the black sheep.

I guess, there is none...

julia> R, x = polynomial_ring(QQ, "x")
(Univariate Polynomial Ring in x over Rational Field, x)

julia> is_power(x^3)
ERROR: MethodError: no method matching is_power(::QQPolyRingElem)

@JohnAAbbott
Copy link
Contributor

Are there use cases for is_power (or similar) for polynomials or ideals?
Related functions are "squarefree factorization" for polynomials (and maybe also polynomial decomposition), and radical for ideals.
I agree that the return order should be consistent! My instinct prefers returning the base first, and the exponent second. I'm not sure why is_prime_power_with_data returns a boolean as well as base, exponent -- that seems superfluous to me (e.g. set base to 0 to indicate not prime?)
CoCoA has a function StarRoot which computes the r from is_power; given n and r it should be easy to compute e (right?) Any suggestions for a better fn name are welcome
Just for self-inconsistency: I prefer that functions whose name is is_XYZ return a boolean (as primary return value). I think that may be why I named the CoCoA function StarRoot rather than IsPower.

@fieker
Copy link
Contributor

fieker commented May 12, 2023 via email

@thofma
Copy link
Collaborator

thofma commented May 28, 2023

@fieker do you have a preference for the order of the return values? Base first or exponent first?

@fieker
Copy link
Contributor

fieker commented May 30, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants