Opened 2 years ago

Closed 2 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) 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 2 years ago by nbruin

You can get the desired result by explicitly doing assume(x,'real'). Is it the case that integer should be implying real? Gaussian integers aren't real ... (but then the algebraic integers aren't discrete in R, so perhaps that's not what integer means).

comment:2 Changed 2 years ago by mforets

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 2 years ago by rws

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.

Last edited 2 years ago by rws (previous) (diff)

comment:4 Changed 2 years ago by rws

See also #20132.

comment:5 Changed 2 years ago by rws

  • 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 2 years ago by rws

  • Branch set to u/rws/doctest_fixes_for_bugs_with_real_part_and_is_real

comment:7 Changed 2 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 593c379b14d26fbfed2baa0c21e7a141e74a1456

New commits:

593c37923093: doctests for some issues with the symbolic real/integer domain

comment:8 Changed 2 years ago by rws

  • 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 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

LGTM. Feel free to set to positive review if this is ready for review.

comment:10 Changed 2 years ago by rws

  • Status changed from new to needs_review

comment:11 Changed 2 years ago by rws

  • Status changed from needs_review to positive_review

comment:12 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

See patchbot

comment:13 Changed 2 years ago by git

  • Commit changed from 593c379b14d26fbfed2baa0c21e7a141e74a1456 to 13954c4451029811b6c5544ac9aec50e6eb383e4

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

58c2a27Merge branch 'develop' into t/23093/doctest_fixes_for_bugs_with_real_part_and_is_real
bd732ec23134: version/chksum/delete old patch
31340ea23134: doctest fixes
c83582fMerge branch 'develop' into t/23134/upgrade_to_pynac_0_7_8
caf7065Merge branch 'u/rws/23199-1' of git://trac.sagemath.org/sage into t/23134/upgrade_to_pynac_0_7_8
e46e07cMerge branch 'u/rws/23199-1' of git://trac.sagemath.org/sage into t/23134/upgrade_to_pynac_0_7_8
5f9a991Merge branch 'develop' into t/23134/upgrade_to_pynac_0_7_8
86f893323134: last minute patch for doc build failures on Debian
20bddeeMerge commit '86f8933d83ba2ac11adeea91e26703b08263f1b4' of git://trac.sagemath.org/sage into t/23093/doctest_fixes_for_bugs_with_real_part_and_is_real
13954c423093: fix doctests

comment:14 Changed 2 years ago by rws

  • 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 2 years ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.