Opened 5 years ago
Closed 4 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 5 years ago by
- Branch set to u/behackl/symbolic/finite-field-reals
- Commit set to a822d5dd9c18b99bcca00138d103e05665247523
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
Patchbot shows doctest fails.
comment:3 Changed 5 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 5 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 5 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 pynac---a fix can be found here: https://github.com/pynac/pynac/pull/160.
comment:6 Changed 5 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 5 years ago by
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 5 years ago by
- Dependencies set to pynac-0.6.5
comment:9 follow-up: ↓ 10 Changed 5 years ago by
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 5 years ago by
Replying to mmezzarobba:
I doubt it is such a good idea to special-case 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 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 5 years ago by
- Status changed from needs_review to needs_work
comment:12 Changed 5 years ago by
- 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 4 years ago by
- Milestone changed from sage-7.1 to sage-duplicate/invalid/wontfix
- Status changed from needs_work to needs_review
comment:14 follow-up: ↓ 15 Changed 4 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 4 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 4 years ago by
sorry, did not see the sage-duplicate/invalid/wontfix milestone.
comment:17 Changed 4 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 4 years ago by
It was added with https://github.com/sagemath/sage/commit/11e2b784 (bottom)
comment:19 Changed 4 years ago by
- 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 4 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