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:  sage8.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 pynac0.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 pynac0.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/231991' of git://trac.sagemath.org/sage into t/23134/upgrade_to_pynac_0_7_8

e46e07c  Merge branch 'u/rws/231991' 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).