Minor Interval fixes

[gschintgen]

References to other Issues or PRs

none

Brief description of what is fixed or changed

Sympy Intervals are purely real and hence ._contains() rejects e.g. S.Infinity. This commit fixes the following two results that were not quite consistent with this definition:

In [1]: c = Symbol('c', real=False)
In [2]: Interval(m, m + 1).contains(c)
Out[2]: c ≥ m ∧ c ≤ m + 1  # should be False
In [3]: e = Symbol('e', extended_real=True)
In [4]: Interval(-oo, oo).contains(e)
Out[4]: True  # is in fact unknown!

The second commit makes Interval reject some obvious non-real intervals.

Other comments

Release Notes

• sets
  • Fixed minor issues in Interval.contains()

[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 minor issues in Interval.contains() (#18426 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
none

#### Brief description of what is fixed or changed
Sympy `Interval`s are purely real and hence `._contains()` rejects e.g. `S.Infinity`. This commit fixes the following two results that were not quite consistent with this definition:
```
In [1]: c = Symbol('c', real=False)
In [2]: Interval(m, m + 1).contains(c)
Out[2]: c ≥ m ∧ c ≤ m + 1  # should be False
In [3]: e = Symbol('e', extended_real=True)
In [4]: Interval(-oo, oo).contains(e)
Out[4]: True  # is in fact unknown!
```

The second commit makes `Interval` reject some obvious non-real intervals.
#### 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 minor issues in `Interval.contains()`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[oscarbenjamin]

Which cases benefit from checking end - start?

Could use fuzzy_and/or/not here (and in other places).

if fuzzy_not(fuzzy_and(i.is_extended_real for s in (start, end, end-start))):

Tri-valued logic is confusing so I like to keep it as close as possible to normal logic:

if not all(is_real(i) for i in (start, end, end-start)):

[gschintgen]

ok, changed.

[oscarbenjamin]

It should not be necessary to check is_real and is_extended_real. The assumptions system computes implications like is_real -> is_extended_real and the contrapositive ! is_extended_real -> ! is_real so these are consistent across all of Expr (actually all of Basic...).

The infinities would all be covered by the is_real is False check as well.

[gschintgen]

The assumptions system computes implications like is_real -> is_extended_real

Thanks for the reminder; changed. (I suppose the explicit checks for the infinities were intended as a kind of micro-optimization, but I guess the assumption system does its best, e.g. caching, to make these quick exits not really worthwhile.)

[oscarbenjamin]

Classes like Infinity have things like is_real = False as class attributes. The ManagedProperties metaclass computes all of the implications of this at class definition time so that is_real becomes a property that returns False. That means that looking up is_real there is probably as efficient as looking up an attribute on S especially if you look up multiple attributes on S.

We should shun using is checks for speed reasons and write code that is simple and clean until an actual profile/benchmark makes the benefit measurable.

[gschintgen]

We should shun using is checks for speed reasons and write code that is simple and clean until an actual profile/benchmark makes the benefit measurable

Agreed. (And thanks for the assumptions internals.)

On a side-note: I was a bit astonished that S.NaN.is_real evaluates to None. I'd have expected False. (I think I can see where it's coming from, but expressions that are yielding NaN are almost by definition neither real nor complex. They may have a limit, but that requires the presence of variables and such.)

[oscarbenjamin]

I think of nan as often representing a failed computation. Possibly calculating in a different way would have given a sensible result but we can't be sure. That means we should not really say anything definitive that it is or is not real etc.

[oscarbenjamin]

x is not None is better than not x is None as there is no ambiguity in the precedence

[codecov]

Codecov Report

Merging #18426 into master will decrease coverage by 11%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##            master    #18426        +/-   ##
==============================================
- Coverage   75.317%   64.317%   -11.001%     
==============================================
  Files          637       637                
  Lines       167069    167070         +1     
  Branches     39417     39417                
==============================================
- Hits        125833    107455     -18378     
- Misses       35702     53521     +17819     
- Partials      5534      6094       +560

[oscarbenjamin]

Looks good. Thanks