Opened 4 years ago
Closed 4 years ago
#23093 closed defect (fixed)
Doctest fixes for bugs with real_part and is_real
Reported by: | mforets | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.0 |
Component: | symbolics | Keywords: | assumptions, var |
Cc: | rws | Merged in: | |
Authors: | Ralf Stephan | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 13954c4 (Commits, GitHub, GitLab) | Commit: | 13954c4451029811b6c5544ac9aec50e6eb383e4 |
Dependencies: | #23134 | Stopgaps: |
Description
The real_part
method of a variable in the integer domain behaves unexpectedly,
sage: SR.var('x', domain='integer').real_part() real_part(x)
compare to
sage: SR.var('x', domain='real').real_part() x
Change History (15)
comment:1 Changed 4 years ago by
comment:2 Changed 4 years ago by
well, true, one can add that as an additional assumption, and I don't know if integer
should be implying real
in the most general case in Sage.
the use case at hand was an expression involving complex exponentials and some variables that are discretized, hence the integer assumption (integers being understood as a subset of the real numbers.)
comment:3 Changed 4 years ago by
Internally in Pynac there are the info flags ìnteger
and cinteger
and the internal bool numeric::is_cinteger()
uses py_is_cinteger
which looks like return py_is_integer(x) or (py_is_crational(x) and py_denom(x) == 1)
, but the functions are buggy (maybe since the parent of I
changed some time ago):
sage: from sage.libs.pynac.pynac import py_is_crational_for_doctest sage: py_is_crational_for_doctest(1) True sage: py_is_crational_for_doctest(2*I) False sage: py_is_cinteger_for_doctest(2*I) False
is_cinteger
is not used in Pynac. For crational I opened the (unrelated to this ticket) https://github.com/pynac/pynac/issues/253 because it's used in Pynac. It's not exposed to the Sage user.
So from the info flag level integer would mean real integer, so this ticket is about a bug in Pynac with setting flags in symbol::set_domain
. I cannot say atm if there are more bugs revealed in consequence.
This also means that the docstring for x.is_integer?
should be adapted to this because Expression.is_integer()
directly queries the integer info flag.
comment:4 Changed 4 years ago by
See also #20132.
comment:5 Changed 4 years ago by
- Dependencies set to pynac-0.7.8
- Report Upstream changed from N/A to Fixed upstream, in a later stable release.
- Summary changed from Real part of integer is unevaluated to Doctest fixes for bugs with real_part and is_real
Fixed in pynac master. I will also add here doctests for the (new) bug sqrt(-2).is_real()
.
comment:6 Changed 4 years ago by
- Branch set to u/rws/doctest_fixes_for_bugs_with_real_part_and_is_real
comment:7 Changed 4 years ago by
- Commit set to 593c379b14d26fbfed2baa0c21e7a141e74a1456
New commits:
593c379 | 23093: doctests for some issues with the symbolic real/integer domain
|
comment:8 Changed 4 years ago by
- Dependencies changed from pynac-0.7.8 to #23134
- Report Upstream changed from Fixed upstream, in a later stable release. to N/A
comment:9 Changed 4 years ago by
- Reviewers set to Travis Scrimshaw
LGTM. Feel free to set to positive review if this is ready for review.
comment:10 Changed 4 years ago by
- Status changed from new to needs_review
comment:11 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:13 Changed 4 years ago by
- Commit changed from 593c379b14d26fbfed2baa0c21e7a141e74a1456 to 13954c4451029811b6c5544ac9aec50e6eb383e4
Branch pushed to git repo; I updated commit sha1. New commits:
58c2a27 | Merge branch 'develop' into t/23093/doctest_fixes_for_bugs_with_real_part_and_is_real
|
bd732ec | 23134: version/chksum/delete old patch
|
31340ea | 23134: doctest fixes
|
c83582f | Merge branch 'develop' into t/23134/upgrade_to_pynac_0_7_8
|
caf7065 | Merge branch 'u/rws/23199-1' of git://trac.sagemath.org/sage into t/23134/upgrade_to_pynac_0_7_8
|
e46e07c | Merge branch 'u/rws/23199-1' of git://trac.sagemath.org/sage into t/23134/upgrade_to_pynac_0_7_8
|
5f9a991 | Merge branch 'develop' into t/23134/upgrade_to_pynac_0_7_8
|
86f8933 | 23134: last minute patch for doc build failures on Debian
|
20bddee | Merge commit '86f8933d83ba2ac11adeea91e26703b08263f1b4' of git://trac.sagemath.org/sage into t/23093/doctest_fixes_for_bugs_with_real_part_and_is_real
|
13954c4 | 23093: fix doctests
|
comment:14 Changed 4 years ago by
- Status changed from needs_work to positive_review
Merging the dependency makes the failing tests pass. However the problem is uncovered that the added doctests change x
to integer, making subsequent tests fail.
So I just merged the dependency and added forget()
.
comment:15 Changed 4 years ago by
- Branch changed from u/rws/doctest_fixes_for_bugs_with_real_part_and_is_real to 13954c4451029811b6c5544ac9aec50e6eb383e4
- Resolution set to fixed
- Status changed from positive_review to closed
You can get the desired result by explicitly doing
assume(x,'real')
. Is it the case thatinteger
should be implyingreal
? Gaussian integers aren't real ... (but then the algebraic integers aren't discrete in R, so perhaps that's not whatinteger
means).