properties of converted finite field elements are wrong

[behackl]

This should not happen:

sage: k.<a> = GF(9)
sage: SR(a).is_real()
True
sage: SR(a).is_positive()
True

Depends on #20312

CC: @rwst

Component: symbolics

Author: Benjamin Hackl

Reviewer: Clemens Heuberger

Issue created by migration from https://trac.sagemath.org/ticket/20162

[behackl]

New commits:

443fbf8	finite field elements are not reals
a822d5d	add doctests for is_real/is_positive

[behackl]

Branch: u/behackl/symbolic/finite-field-reals

[behackl]

Commit: a822d5d

[rwst]

comment:2

Patchbot shows doctest fails.

[behackl]

comment:3

Ah, yes. Maybe I need another way of finding out whether a parent is a finite field; is_finite seems to throw NotImplementedErrors here. That would fix the failures in schemes/elliptic_curves/heigth.py.

However, I'm not sure about the segmentation fault in expression.pyx, calling

sage: (Mod(2,7)*x^2 + Mod(2,7))^7

from sage -gdb prints the line

Program received signal SIGSEGV, Segmentation fault.
GiNaC::add::integer_content (this=0x3019c20) at normal.cpp:325

It seems that something requires the expression to be a real in order for the power to work. This is bad.

Maybe I'll investigate later; have to go now.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

591899b

return false in case of NotImplementedError

[sagetrac-git]

Changed commit from a822d5d to 591899b

[behackl]

comment:5

The last commit fixes the doctest failures in schemes/elliptic_curves/height.py, but not the segmentation fault. Unfortunately, the reason for that lies in pynac---a fix can be found here: pynac/pynac#160.

[videlec]

comment:6

Just to share some related fun with quotients of the integer ring

sage: cos(Zmod(7)(3))
-0.989992496600445
sage: cos(Zmod(10)(3))
1
sage: cos(Zmod(15)(3))
1

[behackl]

comment:7

With pynac-0.6.4 (#20134):

sage: cos(Zmod(7)(3))
-0.989992496600445
sage: cos(Zmod(10)(3))
-0.989992496600445
sage: cos(Zmod(15)(3))
-0.989992496600445

In an ideal world, this would probably raise a ValueError or so. This way it is at least consistent.

[rwst]

Dependencies: pynac-0.6.5

[mezzarobba]

comment:9

I doubt it is such a good idea to special-case finite fields here. Why does py_imag default to returning True in the first place?! And as for returning results based on the parent of the wrapped object, I'd be tempted to base the logic on whether the parent has coercions to things like {Real,Complex}Field(mpfr_prec_min()) instead of having explicit lists of “real” and “non-real” parents.

[behackl]

comment:10

Replying to @mezzarobba:

I doubt it is such a good idea to special-case finite fields here. Why does py_imag default to returning True in the first place?!

This is because if in doubt py_is_real asks py_imag whether the imaginary part is 0---which is the case for these elements, because if in doubt, py_imag assumes that the imaginary part is 0. ;-)

And as for returning results based on the parent of the wrapped object, I'd be tempted to base the logic on whether the parent has coercions to things like {Real,Complex}Field(mpfr_prec_min()) instead of having explicit lists of “real” and “non-real” parents.

This might be an approach worth pursuing.

[rwst]

Changed dependencies from pynac-0.6.5 to #20312

[rwst]

comment:12

Now (with #20312) both flags are false. Can this case be considered to be fixed? Then just a doctest would be needed here.

[rwst]

comment:13

With #22162 this would return Unknown, and a doctest already exist (changed in #22162) so I think this is a duplicate now.

[cheuberg]

comment:14

In any case, this produces a merge conflict.

[rwst]

comment:15

Replying to @cheuberg:

In any case, this produces a merge conflict.

Needs work on a duplicate request!?

[cheuberg]

comment:16

sorry, did not see the sage-duplicate/invalid/wontfix milestone.

[cheuberg]

comment:17

I see that the problem is fixed; can you tell me where the doctest is? I did not see it at first glance.

[rwst]

comment:18

It was added with 11e2b784
(bottom)

[cheuberg]

comment:19

Problem is fixed and doctest has been included in #20475.

[cheuberg]

Reviewer: Clemens Heuberger

[cheuberg]

Changed commit from 591899b to none

[cheuberg]

Changed branch from u/behackl/symbolic/finite-field-reals to none