Opened 4 years ago

Last modified 8 months ago

#18630 needs_review defect

Doctest: Expression.is_positive/negative fixed

Reported by: rws Owned by:
Priority: major Milestone: sage-8.2
Component: symbolics Keywords:
Cc: kcrisman, pelegm Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/18630-1 (Commits) Commit: 8e446e6360353057553e702f188166a7a0817a1e
Dependencies: #24497 Stopgaps:

Description (last modified by rws)

The two functions that query Pynac expressions' info::flags only work with numerics and symbols with domain. The rest simply returns False:

sage: (1-pi).is_negative()
False
sage: (log(1/2)).is_negative()
False
sage: e.is_positive()
False
sage: (e+1).is_positive()
False
sage: (2*e).is_positive()
False
sage: (e^3).is_positive()
False

UPDATE: everything above but the first works now.

Change History (22)

comment:1 Changed 4 years ago by rws

  • Dependencies set to pynac-0.3.9.1
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Fixed upstream, but not in a stable release.

Parts of this are fixed in https://github.com/pynac/pynac/issues/66 (will be pynac-0.3.9.1/0.4.1)

comment:2 Changed 4 years ago by rws

While products are okay already, sums with negative terms are not and will not be for some time. Correct handling depends on https://github.com/pynac/pynac/issues/67 and https://github.com/pynac/pynac/issues/68 and affects #12588 too, for example.

comment:3 Changed 4 years ago by rws

  • Dependencies pynac-0.3.9.1 deleted
  • Description modified (diff)
  • Report Upstream changed from Fixed upstream, but not in a stable release. to Reported upstream. Developers acknowledge bug.

comment:4 Changed 21 months ago by rws

  • Milestone changed from sage-6.8 to sage-8.1
  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to N/A
  • Summary changed from Expression.is_positive/negative incomplete to Doctest: Expression.is_positive/negative fixed

Since everything apart from sums works now, let's look at what is possible with sums containing exact numerics (in the Pynac sense) and constants only. The border case is if the expression is nonzero---but only at certain precision. This is impossible to handle. It is also not possible to convert to float and only check sign if the absolute result is greater some epsilon, because that could be due to precision loss in a denominator.

I'm therefore just adding the doctests here to resolve the ticket.

comment:5 Changed 21 months ago by rws

  • Branch set to u/rws/expression_is_positive_negative_incomplete

comment:6 Changed 21 months ago by rws

  • Authors set to Ralf Stephan
  • Commit set to fe039e29d4e8c45c950722308b35d0188bccfcc3
  • Status changed from new to needs_review

New commits:

fe039e218630: Doctest: Expression.is_positive/negative fixed

comment:7 Changed 16 months ago by git

  • Commit changed from fe039e29d4e8c45c950722308b35d0188bccfcc3 to a643c393571f00b2243d141425920250359da55b

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

a643c39Merge branch 'develop' into t/18630/expression_is_positive_negative_incomplete

comment:8 Changed 15 months ago by rws

  • Status changed from needs_review to needs_work

Docbuild error: ValueError: line 48 of the docstring for sage.symbolic.expression.Expression.is_positive has inconsistent leading whitespace: u'::'

comment:9 Changed 15 months ago by rws

  • Branch changed from u/rws/expression_is_positive_negative_incomplete to u/rws/18630

comment:10 Changed 15 months ago by rws

  • Commit changed from a643c393571f00b2243d141425920250359da55b to 1c62b17993d9894a8b2beee20a63c985665b9304
  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from needs_work to needs_review

New commits:

1c62b1718630: Doctest improvements to ex.is_positive/negative

comment:11 follow-up: Changed 15 months ago by vdelecroix

Would be nice to also have some less trivial examples

sage: (log(1/3) * log(1/2)).is_positive()
True
sage: log((2**500+1)/2**500).is_positive()
True
sage: log(2*500/(2**500-1)).is_negative()
True

And the following are other failing examples

sage: sin(pi/37).is_positive()   # algebraic number!
False
sage: (sqrt(2)-1).is_positive()  # algebric number!
False
sage: log(pi).is_positive()
False 
sage: sin(3).is_positive()
False
sage: sin(5).is_negative()
False

comment:12 follow-up: Changed 15 months ago by vdelecroix

And I don't understand why the following is ok

sage: ((-pi^(1/5))^2).is_positive()
True

while not these ones

sage: (pi^2).is_positive()
False
sage: ((-pi)^2).is_positive()
False

comment:13 in reply to: ↑ 11 ; follow-up: Changed 15 months ago by rws

Replying to vdelecroix:

And the following are other failing examples

sage: sin(pi/37).is_positive()   # algebraic number!
False
sage: (sqrt(2)-1).is_positive()  # algebric number!
False
sage: log(pi).is_positive()
False 
sage: sin(3).is_positive()
False
sage: sin(5).is_negative()
False

They are not failing. The result False simply means "I don't know.".

comment:14 in reply to: ↑ 12 Changed 15 months ago by rws

  • Status changed from needs_review to needs_work

Replying to vdelecroix:

sage: (pi^2).is_positive()
False
sage: ((-pi)^2).is_positive()
False

One logic for b^e.is_positive() is if (e.is_even()) return b.is_real() and b.is_nonzero() but is_nonzero was not implemented for constants until now. This is fixed in https://github.com/pynac/pynac/commit/e92febc468800e4a1457f944bb7a45e9baf0e2f6 and will be in pynac-0.7.15. We can as well put all of these doctests in this ticket too. Thanks for the report!

comment:15 in reply to: ↑ 13 Changed 15 months ago by rws

Replying to rws:

They are not failing. The result False simply means "I don't know.".

See also #22162.

comment:16 Changed 15 months ago by rws

See also https://github.com/pynac/pynac/issues/293 for progress on what is completely implemented as internal Pynac flag.

comment:17 Changed 13 months ago by rws

  • Dependencies set to #24497

comment:18 Changed 13 months ago by rws

  • Branch changed from u/rws/18630 to u/rws/18630-1

comment:19 Changed 13 months ago by rws

  • Commit changed from 1c62b17993d9894a8b2beee20a63c985665b9304 to 8e446e6360353057553e702f188166a7a0817a1e
  • Status changed from needs_work to needs_review

New commits:

8e446e618630: Doctest improvements to ex.is_positive/negative

comment:20 Changed 11 months ago by pelegm

  • Cc pelegm added

comment:21 Changed 8 months ago by pelegm

Seems like the first test still fails:

sage: (1-pi).is_negative()
False

Do we want to merge it as is?

comment:22 Changed 8 months ago by rws

Yes, see comment:15.

Note: See TracTickets for help on using tickets.