Fix bugs in ComplexRegion._contains()

[gschintgen]

References to other Issues or PRs

Fixes #18280

Brief description of what is fixed or changed

The following are fixed in ComplexRegion._contains():

• Complexes and ComplexRegion have incorrect .contains() logic #18280: The method had no provisions for undecidable membership;
  instead of None it would return False.
• Complexes and ComplexRegion have incorrect .contains() logic #18280: The method checked arg(other) for membership in the
  respective theta intervals but those are normalized to [0, 2*pi)
  and not (-pi, pi].
• For PolarComplexRegions the method incorrectly set theta = 0 if
  other was 0. So it returned a somewhat arbitrary truth value in
  this case. Now it will exclusively check complex modulus if other==0.

Tests are added.

Making the tests pass required fixing a number of issues in nonlinsolve's substitution solver. From the commit message:

• _extract_main_soln() used sym as key in complements, but sym is not
  correctly defined at this stage. But the code doesn't raise because an
  unrelated sym variable has been introduced at the very beginning of
  substitution().
• _solve_using_known_values() called _add_intersection_complement() with
  both Intersection=True and Complement=True, but passes only the
  intersection dictionary.
• _add_intersection_complement() could not actually handle intersections
  and complements concurrently.

Other comments

Release Notes

• sets
  • Fixed bugs in ComplexRegion.contains() that could lead to incorrect results when membership is in fact undecidable or at least unknown.
• solvers
  • Fixed nonlinsolve's handling of intersections and complements in the final result.

[sympy-bot]

✅

Hi, I am the SymPy bot (v149). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• sets

  • Fixed bugs in ComplexRegion.contains() that could lead to incorrect results when membership is in fact undecidable or at least unknown. (#18308 by @gschintgen)

• solvers

  • Fixed nonlinsolve's handling of intersections and complements in the final result. (#18308 by @gschintgen)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #18280

#### Brief description of what is fixed or changed
The following are fixed in ComplexRegion._contains():

- #18280: The method had no provisions for undecidable membership;
  instead of `None` it would return `False`.
- #18280: The method checked `arg(other)` for membership in the
  respective theta intervals but those are normalized to [0, 2*pi)
  and not (-pi, pi].
- For PolarComplexRegions the method incorrectly set theta = 0 if
  `other` was 0. So it returned a somewhat arbitrary truth value in
  this case. Now it will exclusively check complex modulus if other==0.

Tests are added.

Making the tests pass required fixing a number of issues in `nonlinsolve`'s `substitution` solver. From the commit message:
- `_extract_main_soln()` used `sym` as key in complements, but `sym` is not
correctly defined at this stage. But the code doesn't raise because an
unrelated `sym` variable has been introduced at the very beginning of
`substitution()`.
- `_solve_using_known_values()` called `_add_intersection_complement()` with
both `Intersection=True` and `Complement=True`, but passes only the
intersection dictionary.
- `_add_intersection_complement()` could not actually handle intersections
and complements concurrently.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* sets
  * Fixed bugs in `ComplexRegion.contains()` that could lead to incorrect results when membership is in fact undecidable or at least unknown.
* solvers
  * Fixed `nonlinsolve`'s handling of intersections and complements in the final result.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[oscarbenjamin]

I see you have a test failure for this

File "/home/travis/virtualenv/python3.8.0/lib/python3.8/site-packages/sympy-1.6.dev0-py3.8.egg/sympy/solvers/solveset.py", line 3286, in sympy.solvers.solveset.nonlinsolve
F
Failed example:
    pprint(nonlinsolve(system, [a, b, c, d]), use_unicode=False)
Expected:
      -1       1               1      -1
    {(---, -d, -, {d} \ {0}), (-, -d, ---, {d} \ {0})}
       d       d               d       d
Got:
       -1                       1                         1                      -
    {({---} \ S.Complexes, -d, {-} \ S.Complexes, {d}), ({-} \ S.Complexes, -d, {-
        d                       d                         d                       
    <BLANKLINE>
    1                       
    --} \ S.Complexes, {d})}
    d  

I've seen this precise example come up in many PRs including my own. It seems to be very flakey.

[gschintgen]

I see you have a test failure for this

File "/home/travis/virtualenv/python3.8.0/lib/python3.8/site-packages/sympy-1.6.dev0-py3.8.egg/sympy/solvers/solveset.py", line 3286, in sympy.solvers.solveset.nonlinsolve
F
Failed example:
    pprint(nonlinsolve(system, [a, b, c, d]), use_unicode=False)
Expected:
      -1       1               1      -1
    {(---, -d, -, {d} \ {0}), (-, -d, ---, {d} \ {0})}
       d       d               d       d
Got:
       -1                       1                         1                      -
    {({---} \ S.Complexes, -d, {-} \ S.Complexes, {d}), ({-} \ S.Complexes, -d, {-
        d                       d                         d                       
    <BLANKLINE>
    1                       
    --} \ S.Complexes, {d})}
    d  

I've seen this precise example come up in many PRs including my own. It seems to be very flakey.

It's flaky indeed. It's the one I meant here and that got me to investigate.

I noticed that nonlinsolve has some broken logic:

sympy/sympy/solvers/solveset.py

Lines 2759 to 2771 in 8a7dc8e

	for key_sym, value_sym in sym_set.items():
	if key_sym == key_res:
	if intersection_true:
	# testcase is not added for this line(intersection)
	new_value = \
	Intersection(FiniteSet(value_res), value_sym)
	if new_value is not S.EmptySet:
	res_copy[key_res] = new_value
	if complements_true:
	new_value = \
	Complement(FiniteSet(value_res), value_sym)
	if new_value is not S.EmptySet:
	res_copy[key_res] = new_value

The above code block is called with both intersection_true and complement_true True, so that the second block overrides the first one...

The commit I pushed just now handles almost all the other test failures. But there's another remaining test failure that pointed me to a very unfortunate fact: Many tests in test_solveset.py assume x and y to have been created beforehand, which is true indeed, but they're actually instantiated with real=True assumption! I don't believe that's intentional and it could falsify a lot of tests...

sympy/sympy/solvers/tests/test_solveset.py

Lines 47 to 55 in 8a7dc8e

	a = Symbol('a', real=True)
	b = Symbol('b', real=True)
	c = Symbol('c', real=True)
	x = Symbol('x', real=True)
	y = Symbol('y', real=True)
	z = Symbol('z', real=True)
	q = Symbol('q', real=True)
	m = Symbol('m', real=True)
	n = Symbol('n', real=True)

[oscarbenjamin]

I don't believe that's intentional and it could falsify a lot of tests...

Blame shows that it dates back to the creation of the solveset module.

[gschintgen]

Strange: With the latest commit the number of Travis test failures in test_solveset went up instead of down. The failures on Travis all mention _timeout so I thought that maybe there was a Travis hiccup, so I restarted this specific job, but the results are the same. I'm also now noticing that in general tests are started via a _timeout function so that's normal.

What's bothering me is that when testing locally I only have a single failure (test_solve_polynomial_symbolic_param) with test("solveset"). I also ran the two first failing Travis tests manually on my local setup and everything was fine.

[gschintgen]

I also ran the two first failing Travis tests manually on my local setup and everything was fine.

Never mind. I think I figured it out. I had a closer look at the Travis logs and it seems what is actually tested is not the PR branch but a "fictitious" merged branch. That's sensible I guess.

[oscarbenjamin]

Yes, Travis tests the PR after merging it into master (that's why the tests don't run when there's a merge conflict). That should mean that conflicting PRs can't lead to test failures on master but there is a loop-hole: if the tests have passed then other PRs can be merged to master and the tests won't re-run but the PR remains mergeable.

[oscarbenjamin]

I can remember being annoyed by this test. I made sure to leave a comment so that others wouldn't worry too much about it.

[codecov]

Codecov Report

Merging #18308 into master will increase coverage by 0.001%.
The diff coverage is 66.666%.

@@              Coverage Diff              @@
##            master    #18308       +/-   ##
=============================================
+ Coverage   75.307%   75.308%   +0.001%     
=============================================
  Files          639       639               
  Lines       167125    167124        -1     
  Branches     39418     39420        +2     
=============================================
+ Hits        125857    125859        +2     
+ Misses       35731     35720       -11     
- Partials      5537      5545        +8

[oscarbenjamin]

Looks good!