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

Allow python ints in sage.groups.generic.has_order #38980

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

DaveWitteMorris
Copy link
Member

Fixes #38708.

We revise sage.groups.generic.has_order to allow the argument n to be any integer (including a python int), not only a sage integer.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

None.

Copy link

github-actions bot commented Nov 17, 2024

Documentation preview for this PR (built with commit f20bed4; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@DaveWitteMorris DaveWitteMorris marked this pull request as draft November 17, 2024 05:42
@DaveWitteMorris
Copy link
Member Author

This PR causes a doctest failure:

sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/schemes/elliptic_curves/ell_point.py
**********************************************************************
File "src/sage/schemes/elliptic_curves/ell_point.py", line 589, in sage.schemes.elliptic_curves.ell_point.?.has_order
Failed example:
    E(0).has_order(Factorization([]))
Exception raised:
    Traceback (most recent call last):
      File "/Users/dmorris/development/sage/src/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/dmorris/development/sage/src/sage/doctest/forker.py", line 1136, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.elliptic_curves.ell_point.?.has_order[12]>", line 1, in <module>
        E(Integer(0)).has_order(Factorization([]))
      File "/Users/dmorris/development/sage/src/sage/schemes/elliptic_curves/ell_point.py", line 596, in has_order
        ret = generic.has_order(self, n, operation='+')
      File "/Users/dmorris/development/sage/src/sage/groups/generic.py", line 1516, in has_order
        n = integer_ring.ZZ(n)
      File "sage/structure/parent.pyx", line 901, in sage.structure.parent.Parent.__call__
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 164, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_
        raise
      File "sage/structure/coerce_maps.pyx", line 159, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_
        return C._element_constructor(x)
      File "sage/rings/integer.pyx", line 723, in sage.rings.integer.Integer.__init__
        raise TypeError("unable to coerce %s to an integer" % type(x))
    TypeError: unable to coerce <class 'sage.structure.factorization.Factorization'> to an integer

@DaveWitteMorris
Copy link
Member Author

The documentation states that the input should be either an integer or in the class sage.structure.factorization.Factorization. The original PR required a more specific type of factorization, which caused the doctest failure. My bad.

@DaveWitteMorris DaveWitteMorris marked this pull request as ready for review November 17, 2024 06:20
@user202729
Copy link
Contributor

user202729 commented Nov 17, 2024

Not sure if ZZ(...) is a good idea. Most of the code I can see use isinstance() instead.

Ouch! No, don't do that!! QQ(pi.n()) yields a rational number! Conversion can really mean "is there any way in which you could interpret this as an element of ..."? Perhaps try to coerce in to QQ, but in this case you're really looking at the "action" of QQ via exponentiation. There could be other actions. Can't you just let the coercion system figure out what type your exponent should have?

#38362 (comment)

Though for ZZ instead of QQ I'm not sure if there's any bad side effect.

What about ZZ.coerce(value) instead?

@DaveWitteMorris
Copy link
Member Author

I think ZZ(the input) is a pretty standard idiom for situations such as this where the input is supposed to be an integer.

Copy link
Member

@yyyyx4 yyyyx4 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@DaveWitteMorris
Copy link
Member Author

Thanks for the review!

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

Successfully merging this pull request may close these issues.

has_order() errors out when the given order has type int
3 participants