Opened 3 years ago

Closed 2 years ago

#20162 closed defect (invalid)

properties of converted finite field elements are wrong

Reported by: behackl Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: symbolics Keywords:
Cc: rws Merged in:
Authors: Benjamin Hackl Reviewers: Clemens Heuberger
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #20312 Stopgaps:

Description

This should not happen:

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

Change History (20)

comment:1 Changed 3 years ago by behackl

  • Branch set to u/behackl/symbolic/finite-field-reals
  • Commit set to a822d5dd9c18b99bcca00138d103e05665247523
  • Status changed from new to needs_review

New commits:

443fbf8finite field elements are not reals
a822d5dadd doctests for is_real/is_positive

comment:2 Changed 3 years ago by rws

Patchbot shows doctest fails.

comment:3 Changed 3 years ago by behackl

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.

comment:4 Changed 3 years ago by git

  • Commit changed from a822d5dd9c18b99bcca00138d103e05665247523 to 591899ba8eda202cf8af3a2c22ef7aab85059ccf

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

591899breturn false in case of NotImplementedError

comment:5 Changed 3 years ago by behackl

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: https://github.com/pynac/pynac/pull/160.

comment:6 Changed 3 years ago by vdelecroix

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

comment:7 Changed 3 years ago by behackl

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.

comment:8 Changed 3 years ago by rws

  • Dependencies set to pynac-0.6.5

comment:9 follow-up: Changed 3 years ago by mmezzarobba

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.

comment:10 in reply to: ↑ 9 Changed 3 years ago by behackl

Replying to mmezzarobba:

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.

comment:11 Changed 3 years ago by behackl

  • Status changed from needs_review to needs_work

comment:12 Changed 3 years ago by rws

  • Dependencies changed from pynac-0.6.5 to #20312

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

comment:13 Changed 2 years ago by rws

  • Milestone changed from sage-7.1 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

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

comment:14 follow-up: Changed 2 years ago by cheuberg

  • Status changed from needs_review to needs_work

In any case, this produces a merge conflict.

comment:15 in reply to: ↑ 14 Changed 2 years ago by rws

  • Status changed from needs_work to needs_review

Replying to cheuberg:

In any case, this produces a merge conflict.

Needs work on a duplicate request!?

comment:16 Changed 2 years ago by cheuberg

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

comment:17 Changed 2 years ago by cheuberg

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

comment:19 Changed 2 years ago by cheuberg

  • Branch u/behackl/symbolic/finite-field-reals deleted
  • Commit 591899ba8eda202cf8af3a2c22ef7aab85059ccf deleted
  • Reviewers set to Clemens Heuberger
  • Status changed from needs_review to positive_review

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

comment:20 Changed 2 years ago by vbraun

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.