sympy/core: Basic and Expr __eq__ are merged #22651
Conversation
|
✅ Hi, I am the SymPy bot (v162). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
|
Not really sure why this makes |
Maybe add something that detects when |
|
Alright, looks like this comparison in |
|
I thought I had removed that line but apparently I just left a comment: Lines 395 to 399 in 81cd263 For the problem at hand though I guess trace it back a bit. Why is this comparing a set with float boundaries against one with rational boundaries? |
|
Because it obtains a rational interval from |
Maybe it shouldn't be expected to work then. Where is the test? |
This method should probably be made to either return a default value or raise an error if no if branch returns anything. |
Yes, this shouldn't be expected to work. It is similar to the other failures, I got confused due to the |
|
If there are tests that only fail on PyPy then that implies that we have a difference of behaviour somehow between SymPy running on PyPy and SymPy running on CPython. That's a bug in one of SymPy, PyPy or CPython that should be reported or fixed. How exactly can the difference in behaviour be reproduced? |
|
I can't get any useful output on this unfortunately. |
|
Looks like the problem in optional-dependencies is not due to this PR, which explains why I couldn't figure out why these changes cause that. |
ThePauliPrinciple
force-pushed
the
basic_expr_eq_merge
branch
from
811d385
to
d2139ce
Compare
|
There really seems to be something buggy about this. Now it fails in 3.10, but doesn't fail in in the other tests (for as far as they run). Locally it fails for me: assert is_strictly_decreasing(1/(x**2 - 3*x), Interval.open(1.5, 3))It returns |
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) before after ratio
[467f7455] [8da64011]
- 205±0.8ms 135±0.7ms 0.66 large_exprs.TimeLargeExpressionOperations.time_subs
- 210±0.2μs 104±0.4μs 0.50 matrices.TimeMatrixExpression.time_MatMul
- 13.3±0.01ms 7.87±0.3ms 0.59 matrices.TimeMatrixExpression.time_MatMul_doit
Significantly changed benchmark results (master vs previous release) before after ratio
[907895ac] [467f7455]
+ 6.79±0.04ms 10.2±0.3ms 1.51 matrices.TimeMatrixPower.time_Case1
- 4.16±0.01s 316±2ms 0.08 polygon.PolygonArbitraryPoint.time_bench01
+ 3.29±0ms 5.70±0.06ms 1.73 solve.TimeMatrixOperations.time_det(4, 2)
+ 3.29±0.02ms 5.76±0.04ms 1.75 solve.TimeMatrixOperations.time_det_bareiss(4, 2)
+ 37.3±0.2ms 68.9±0.2ms 1.85 solve.TimeMatrixSolvePyDySlow.time_linsolve(1)
+ 37.6±0.1ms 69.3±0.4ms 1.84 solve.TimeMatrixSolvePyDySlow.time_solve(1)
Full benchmark results can be found as artifacts in GitHub Actions |
|
Surely the reported speedups aren't caused by this PR? |
|
Looks like the result is actually non-deterministic: Note that during one run it is printing |
|
The bug seems to boil down to list(ordered(set([Interval.open(Rational(3,2), 3), Interval.open(1.50000000000000, 3)])))being underterministic. Then apparently one order leads to a different result compared to the other order when those two are not equal (because 3/2 is now not equal to 1.5 but this problem is not there when they are equal). This happens in the Lines 395 to 400 in 81cd263 is_subset.
Not yet sure how to fix this or what is actually the reason why this ordering is important. |
|
I've seen this before. The In [1]: list(ordered([S.Half, 0.5]))
Out[1]: [1/2, 0.5]
In [2]: list(ordered([0.5, S.Half]))
Out[2]: [0.5, 1/2]The ultimate fix for that is #20033 but it could be possible to add a workaround for in the mean time. |
|
|
||
| def test_is_monotonic(): | ||
| """Test whether is_monotonic returns correct value.""" | ||
| assert is_monotonic(1/(x**2 - 3*x), Interval.open(1.5, 3)) | ||
| assert is_monotonic(1/(x**2 - 3*x), Interval.open(Rational(3,2), 3)) | ||
| assert is_monotonic(1/(x**2 - 3*x), Interval.open(1.5, 3)) is None |
There was a problem hiding this comment.
Changes like this make me hesitant to accept that making rationals and floats unequal is actually a good idea (see this comment). 1.5 is exactly equal to 3/2 as a floating point number. The answer here is fortunately not wrong, just not as helpful anymore, but one could imagine scenarios where this breaks things in a more serious way.
This is obviously more of a question for #20033 than here. This PR just makes things consistent, which is good, and the changes are pretty minimal that it wouldn't be a big deal to revert them if we decide to go the other way.
There was a problem hiding this comment.
Perhaps we can get the best of both worlds in this case. The mathematical question here is whether the expr is "monotonic" on a certain interval and it most certainly is monotonic along the given Float interval. So this function should deal with comparing based on "value equivalence" rather than "structural equivalence". So I think in is_subset
Lines 399 to 400 in 7c0fa07
might be a wrong use of
== and should use a comparison based on value.
|
If this is the prefered solution, I do believe that the tests should reflect this with checks for both rational and float intervals. |
|
Alright looks like this idea doesn't actually work and I was passing tests by coincidence. Considering that currently this PR does not yield consistent results and I don't know how to make it do so, I think I want to close this, unless there are any suggestions to make this update work with |
|
This surely is a problem: >>> fi=Interval(3/2,oo)
>>> ri=Interval(Rational(3,2),oo)
>>> for i in (fi, ri):
... for u in ((fi, ri), (ri, fi)):
... print(i, u)
... i.is_subset(Union(*u))
...
Interval(1.50000000000000, oo) (Interval(1.50000000000000, oo), Interval(3/2, oo))
True
Interval(1.50000000000000, oo) (Interval(3/2, oo), Interval(1.50000000000000, oo))
True
Interval(3/2, oo) (Interval(1.50000000000000, oo), Interval(3/2, oo))
Interval(3/2, oo) (Interval(3/2, oo), Interval(1.50000000000000, oo))how can a be a subset of the union of a and b, but b not be a subset of the union of a and b? |
|
The issue with floats is basically that we don't know the origin of the floats so e.g.: In [25]: a = Rational(3, 2) + Rational(1, 10**20)
In [26]: a
Out[26]:
150000000000000000001
─────────────────────
100000000000000000000
In [27]: i = Interval(a, 2)
In [28]: ie = i.evalf()
In [29]: i
Out[29]:
⎡150000000000000000001 ⎤
⎢─────────────────────, 2⎥
⎣100000000000000000000 ⎦
In [30]: ie
Out[30]: [1.5, 2.0]
In [31]: Rational(3, 2) in i
Out[31]: False
In [32]: Rational(3, 2) in ie
Out[32]: TrueThe way I think about this is that where I have a float that is equal to some exact rational number I imagine that the float represents a range of values around that number (an open set). If the statement isn't true for an arbitrarily small open set then SymPy shouldn't give True. With that interpretation I am not sure if the test results that you are currently worrying about should be expected to pass. |
|
I managed to break down the issue more fundamentally, with >>> Interval(1.5,oo).intersect(Interval(Rational(3,2),oo)) == Interval(Rational(3,2),oo).intersect(Interval(3/2,oo))
FalseI do believe it should not matter for intersect what the order is, but it does now (it chooses >>> Interval(Rational(3,2),oo).intersect(Interval(3/2,oo))
Interval(3/2, oo)
>>> Interval(3/2,oo).intersect(Interval(Rational(3,2),oo))
Interval(1.50000000000000, oo) |
|
Should those intersects return |
|
Still doesn't solve it. Since I can't get a deterministic result I'm closing this. |
I'll give this some more thought... |
|
Something like this seems to work: diff --git a/sympy/core/sorting.py b/sympy/core/sorting.py
index f255bbf..6ba314e 100644
--- a/sympy/core/sorting.py
+++ b/sympy/core/sorting.py
@@ -172,6 +172,8 @@ def _node_count(e):
# some object has a non-Basic arg, it needs to be
# fixed since it is intended that all Basic args
# are of Basic type (though this is not easy to enforce).
+ if e.is_Float:
+ return 0.5
return 1 + sum(map(_node_count, e.args))
diff --git a/sympy/sets/sets.py b/sympy/sets/sets.py
index 4e88ebd..d17658f 100644
--- a/sympy/sets/sets.py
+++ b/sympy/sets/sets.py
@@ -2463,7 +2463,7 @@ def simplify_intersection(args):
args = set(args)
new_args = True
while new_args:
- for s in args:
+ for s in ordered(args):
new_args = False
for t in args - {s}:
new_set = intersection_sets(s, t) |
|
Do I understand correctly that this would change diff --git a/sympy/sets/handlers/intersection.py b/sympy/sets/handlers/intersection.py
index 1980251c5d..fc31545494 100644
--- a/sympy/sets/handlers/intersection.py
+++ b/sympy/sets/handlers/intersection.py
@@ -426,7 +426,7 @@ def intersection_sets(a, b): # noqa:F811
start = a.start
left_open = a.left_open
else:
- start = a.start
+ start = ordered([a,b])[0].start
left_open = a.left_open or b.left_open
if a.end < b.end:
@@ -436,7 +436,7 @@ def intersection_sets(a, b): # noqa:F811
end = b.end
right_open = b.right_open
else:
- end = a.end
+ end = ordered([a,b])[0].end
right_open = a.right_open or b.right_open
if end - start == 0 and (left_open or right_open):so that |
We could do both. I was just looking at the source of the nondeterminism in this particular example. |
|
This still needs cleanup |
ThePauliPrinciple
force-pushed
the
basic_expr_eq_merge
branch
from
6c59d8b
to
48e6751
Compare
|
This is ready for review again. |
|
Looks good. |
|
Actually the release note needs clarification. Can you edit it here? If we say that Basic now behaves the same as Expr then what does that mean? How were they different and what is changed to make them the same? |
Is it acceptable now? |
|
Yes, looks good. |
References to other Issues or PRs
Helps with #22607
Brief description of what is fixed or changed
Currently,
Exprensures thatExpr(S(1)) != Expr(S(1.0)). However, this does not work forBasic:Basic(S(1)) == Basic(S(1.0)). Since this is the only difference currently betweenBasicandExpr's__eq__method, this PR attempts to merge these two so that__eq__only has to be defined inBasic.This causes backwards compatibility issues, but I believe
Basic(S(1)) == Basic(S(1.0))should be considered a bug.
Other comments
Release Notes
Basicnow behaves the same asExprwhen comparing structural equality with==.