10248: watch for non-FiniteSets when iterating an intersection #10249
Conversation
| return (x for x in s if x in other) | ||
| for x in s: | ||
| c = other.contains(x) | ||
| if c == True: |
There was a problem hiding this comment.
c is True and c is False are much faster. if bool(c): or if bool(c) is True are probably better if you want to test equivalence.
There was a problem hiding this comment.
bool will convert unevaluate contains results to True; fuzzy_bool can be used but then we are using a function call to convert c to True so we can use is to test it. My hunch is that this will be slower than just using the equality check.
My timeit runs show that for t = S.true, fuzzy_bool(t) == True and fuzzy_bool(t) is True essentially take the same time.
So until we do an audit and make sure that all contains routines return S.true, S.false, or an expression, it will be safer to use == True to test for S.true or True.
There was a problem hiding this comment.
we do an audit
I did a grep for places where contains is followed by an == and changed those. I found a few others by looking for where = is followed by contains to see if a variable was used to store the result and then checked to see how it was used. I included timing info in the commit message.
If there are no objections and tests pass I will commit in about 24 h.
Although it involves a function call, sympifying a True value
and comparing like `is S.true` is significantly faster than
comparing with `==`.
>>> timeit('S.true == True',...)
12.895716891578054
>>> timeit('S.true == sympify(True)',...)
5.6353503070422306
>>> timeit('S.true is sympify(True)',...)
2.1583746294627275
10248: watch for non-FiniteSets when iterating an intersection
|
|
||
| def test_issue_10248(): | ||
| assert list(Intersection(S.Reals, FiniteSet(x))) == [ | ||
| And(x < oo, x > -oo)] |
There was a problem hiding this comment.
Why does this produce a Boolean? The elements of Intersection(S.Reals, FiniteSet(x)) are numbers, not booleans.
| elif c is S.false: | ||
| pass | ||
| else: | ||
| yield c |
There was a problem hiding this comment.
I think it would be better if it raised ValueError: Cannot determine if x is contained in the set during iteration here.
…when iterating an intersection This is similar to calling bool() on an undetermined relational (although in that case we get TypeError instead of ValueError, I'm not sure which is better here). See sympy#10249.
fixes #10248