sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.46k stars 482 forks source link

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

Open DaveWitteMorris opened 5 days ago

DaveWitteMorris commented 5 days ago

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.

:memo: Checklist

:hourglass: Dependencies

None.

github-actions[bot] commented 5 days ago

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

DaveWitteMorris commented 5 days ago

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 commented 5 days ago

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.

user202729 commented 4 days ago

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?

https://github.com/sagemath/sage/pull/38362#issuecomment-2325689726

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

What about ZZ.coerce(value) instead?

DaveWitteMorris commented 4 days ago

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

DaveWitteMorris commented 3 days ago

Thanks for the review!