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:  sageduplicate/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
 Branch set to u/behackl/symbolic/finitefieldreals
 Commit set to a822d5dd9c18b99bcca00138d103e05665247523
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
Patchbot shows doctest fails.
comment:3 Changed 3 years ago by
Ah, yes. Maybe I need another way of finding out whether a parent is a finite field; is_finite
seems to throw NotImplementedError
s 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
 Commit changed from a822d5dd9c18b99bcca00138d103e05665247523 to 591899ba8eda202cf8af3a2c22ef7aab85059ccf
Branch pushed to git repo; I updated commit sha1. New commits:
591899b  return false in case of NotImplementedError

comment:5 Changed 3 years ago by
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 pynaca fix can be found here: https://github.com/pynac/pynac/pull/160.
comment:6 Changed 3 years ago by
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
With pynac0.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
 Dependencies set to pynac0.6.5
comment:9 followup: ↓ 10 Changed 3 years ago by
I doubt it is such a good idea to specialcase 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 “nonreal” parents.
comment:10 in reply to: ↑ 9 Changed 3 years ago by
Replying to mmezzarobba:
I doubt it is such a good idea to specialcase finite fields here. Why does
py_imag
default to returningTrue
in the first place?!
This is because if in doubt py_is_real
asks py_imag
whether the imaginary part is 0which 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 “nonreal” parents.
This might be an approach worth pursuing.
comment:11 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:12 Changed 3 years ago by
 Dependencies changed from pynac0.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
 Milestone changed from sage7.1 to sageduplicate/invalid/wontfix
 Status changed from needs_work to needs_review
comment:14 followup: ↓ 15 Changed 2 years ago by
 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
 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
sorry, did not see the sageduplicate/invalid/wontfix milestone.
comment:17 Changed 2 years ago by
I see that the problem is fixed; can you tell me where the doctest is? I did not see it at first glance.
comment:18 Changed 2 years ago by
It was added with https://github.com/sagemath/sage/commit/11e2b784 (bottom)
comment:19 Changed 2 years ago by
 Branch u/behackl/symbolic/finitefieldreals 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
 Resolution set to invalid
 Status changed from positive_review to closed
New commits:
finite field elements are not reals
add doctests for is_real/is_positive