update ConditionSet #19512
update ConditionSet #19512
Conversation
|
✅ Hi, I am the SymPy bot (v160). 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.7. 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.
Update The release notes on the wiki have been updated. |
|
Should ConditionSet have limitations like Lambda on the structure of the first argument in terms of how many times a symbol can appear? >>> ConditionSet((x,(x,y)),x<y,Reals*Reals**2)
ConditionSet((x, (x, y)), x < y, ProductSet(Reals, ProductSet(Reals, Reals)))
>>> Lambda((x,(x,y)),x+y)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "sympy\core\function.py", line 1964, in __new__
cls._check_signature(sig)
File "sympy\core\function.py", line 1990, in _check_signature
rcheck(sig)
File "sympy\core\function.py", line 1982, in rcheck
rcheck(a)
File "sympy\core\function.py", line 1979, in rcheck
raise BadSignatureError("Duplicate symbol %s" % a)
sympy.core.function.BadSignatureError: Duplicate symbol x
>>> Lambda((x,(z,y)),x+y)
Lambda((x, (z, y)), x + y) |
smichr
force-pushed
the
sol-cond
branch
2 times, most recently
from
3899d96
to
6a57b17
Compare
Yes I think so. There is another problem: In [4]: C = ConditionSet((x, y), x<y, Reals**2)
In [5]: C
Out[5]:
⎧ 2 ⎫
⎨(x, y) | (x, y) ∊ ℝ ∧ (x < y)⎬
⎩ ⎭
In [6]: (1, 2) in C
---------------------------------------------------------------------------
BadArgumentsError That can be fixed with diff --git a/sympy/sets/conditionset.py b/sympy/sets/conditionset.py
index 61972eb609..63d710553b 100644
--- a/sympy/sets/conditionset.py
+++ b/sympy/sets/conditionset.py
@@ -201,7 +201,7 @@ def bound_symbols(self):
def _contains(self, other):
return And(
Contains(other, self.base_set),
- Lambda(self.sym, self.condition)(other))
+ Lambda((self.sym,), self.condition)(other))
def as_relational(self, other):
return And(Lambda(self.sym, self.condition)(That does make me wonder if though if |
But should it have anything other than a flat structure? I can understand Lambda having structure to match how one is expecting to pass the arguments, but am not sure that such is needed for ConditionSet. |
|
The thing is that the signature for the ConditionSet also specifies the structure of the objects represented by the set. So e.g. In [242]: ConditionSet((x, (y, z)), (x<y) & (y<z), Reals*Reals**2)
Out[242]:
⎧ ⎛ 2⎞ ⎫
⎨(x, (y, z)) | (x, (y, z)) ∊ ℝ × ⎝ℝ ⎠ ∧ (False)⎬
⎩ ⎭This represents a subset of the set It isn't strictly necessary since we can always use an extra ImageSet to rearrange the shape e.g.: In [4]: C = ConditionSet((x, y, z), (x<y) & (y<z), Reals*Reals**2)
In [5]: I = ImageSet(Lambda(((x, y, z),), (x, (y, z))), C)
In [6]: I
Out[6]:
⎧ ⎧ ⎛ 2⎞ ⎫⎫
⎨(x, (y, z)) | (x, y, z) ∊ ⎨(x, y, z) | (x, y, z) ∊ ℝ × ⎝ℝ ⎠ ∧ (x < y ∧ y < z)⎬⎬
⎩ ⎩ ⎭⎭It might be the case though that it's easier to manipulate ImageSet and ConditionSet together if they have interchangeable args. |
|
I don't think assumptions on dummy args should be supported or else it confounds the meaning of the set: But that set should be "all integers less than 2". Is that a garbage-in-garbage-out result? |
smichr
force-pushed
the
sol-cond
branch
4 times, most recently
from
307451c
to
9a77514
Compare
Codecov Report
@@ Coverage Diff @@
## master #19512 +/- ##
=============================================
- Coverage 75.682% 75.668% -0.014%
=============================================
Files 657 657
Lines 170378 170405 +27
Branches 40174 40182 +8
=============================================
- Hits 128946 128943 -3
- Misses 35801 35817 +16
- Partials 5631 5645 +14 |
|
I have not added any condition checking with symbols and now allow substitutions which were formerly ignored. I marked some questions with XXX in the added code. |
smichr
force-pushed
the
sol-cond
branch
2 times, most recently
from
be61cdc
to
d524f81
Compare
smichr
force-pushed
the
sol-cond
branch
3 times, most recently
from
9ecb24b
to
3a8d82a
Compare
|
There is now 100% nominal converage of conditionset.py |
|
@oscarbenjamin , if you want to give this a look again, I think tests will be passing |
smichr
force-pushed
the
sol-cond
branch
4 times, most recently
from
408bdea
to
6b78e0a
Compare
|
|
||
| """ | ||
| def __new__(cls, sym, condition, base_set=S.UniversalSet): |
There was a problem hiding this comment.
I'm not sure what to do about this: if I make the signature like ImageSet and allow multiple sets to correspond to multiple args then we lose the keyword base_set since even if I made the signature (cls, sym, condition, *base_set) I couldn't pass the sets by name (e.g. ...base_set=Reals) only by position (as I understand it).
There was a problem hiding this comment.
It would be possible by having *args and **kwargs and manually checking if a keyword argument was supplied.
I'm not sure whether it's a good idea though. I wonder if the multi-arg form of ImageSet is a bad idea as it does complicate the definition. It makes some sense for ImageSet because it allows to matches a multi-arg Lambda. I think it definitely makes less sense for CondtionSet though as it would break some symmetries in the n-ary case:
ConditionSet(x, p(x), S1, S2, S3) = {x : p(x), x in prod(S1, S2, S3)}
ConditionSet(x, p(x), S1, S2) = {x : p(x), x in prod(S1, S2)}
ConditionSet(x, p(x), S1) = {x : p(x), x in S1}
Here the first two are sets of 3-tuples and 2-tuples. Shouldn't the third be a set of 1-tuples?
We don't need to keep the ImageSet in this form though since there is always an alternative. I meant to add a method to ImageSet (not sure if I did) that would convert it to single base set form, something like:
>>> ImageSet(Lambda((x, y, z), x+y+z, S1, S2, S3).canonical()
ImageSet(Lambda(((x, y, z),), x+y+z, ProductSet(S1, S2, S3))
Perhaps in code that wants to relate ImageSet and ConditionSet we should just always call the canonical method. Maybe canonical isn't a good name and it could be flatten (or unflatten?) or a property rather than a method. Not sure... Maybe ImageSet should automatically canonicalise itself... or maybe doit should?
There was a problem hiding this comment.
ConditionSet(x, p(x), S1, S2, S3) = {x : p(x), x in prod(S1, S2, S3)}
The way I have it set now, this would raise an error since x is passed; it would not raise an error for ConditionSet((x,y,z), p(x,y,z), S!, S2, S3). I don't think we would want to allow the form you gave to pass or else how would the individual components of x be used in the predicate?
There was a problem hiding this comment.
Does that not leave an ambiguity:
ConditionSet((x, y, z), p(x, y, z), S)
ConditionSet((x, y, z), p(x, y, z), S1, S2, S3)
Then in the first case (x, y, z) is an element of S. In the second case it is an element of prod(S1, S2, S3). Then in the n-ary case we should have
syms = tuple(Dummy() for _ in range(n))
pred = sum(syms) < 0
basesets = [Reals] * n
ConditionSet(syms, pred, *basesets)
We have that syms represents an element of ProductSet(*basesets). That's fine and for case 1 that gives
ConditionSet((x,), x<0, Reals)
However that's now inconsistent with
ConditionSet(x, x<0, Reals)
because here x is not an element of Reals**1.
If we disallow the multiple base sets then we can have a consistent definition that
ConditionSet(x, p, X)
ConditionSet((x,), p, ProductSet(X))
ConditionSet((x, y), p, ProductSet(X, Y))
ConditionSet((x, y, z), p, ProductSet(X, Y, Z))
Here the first argument to ConditionSet always represents an element of the 3rd argument.
There was a problem hiding this comment.
ConditionSet canonicalizes the base set so ...S1, S2, S3 is interpreted (and stored) as ProductSet(S1, S2, S3).
There was a problem hiding this comment.
A FiniteSet can have anything including tuples and non-tuples and tuples of mixed length
I suppose there could be some sort of predicate that would not require uniformly shaped elements in a FiniteSet...that seems a little obtuse to me, however.
There was a problem hiding this comment.
There are lots of good changes here but I think it would be better to stick to only having a single base set and not modify the base set to make a ProductSet in ConditionSet((x, y), x<0, Integers).
some sort of predicate that would not require uniformly shaped elements
You could have something like:
In [1]: ConditionSet((x, y), x<0, {(1, (2, 3)), (4, (5, 6, 7))})
Out[1]: {(x, y) | (x, y) ∊ {(1, (2, 3)), (4, (5, 6, 7))} ∧ (x < 0)}In this PR that comes out strangely:
In [2]: ConditionSet((x, y), x<0, {(1, (2, 3)), (4, (5, 6, 7))})
Out[2]:
⎧ 2 ⎫
⎨(x, y) | (x, y) ∊ {(1, (2, 3)), (4, (5, 6, 7))} ∧ (x < 0)⎬
⎩ ⎭There was a problem hiding this comment.
not modify the base set to make a ProductSet
You could have something like:
In [1]: ConditionSet((x, y),
So should we just let errors rise when the user tries to do anything with the set?
There was a problem hiding this comment.
So should we just let errors rise when the user tries to do anything with the set?
I think for now it's fine to do that. The most important thing to pin down is what exactly is the definition of ConditionSet when given valid arguments.
If we wanted to handle the invalid arguments better at the outset we could add some kind of is_tuple_set property to sets that tells you whether it could possibly be a tuple in general (It could be always True for ProductSet, always None for FiniteSet, False for most atomic sets, fuzzy_and for Union, fuzzy_or? for Intersection). We know e.g. that Integers does not contain any tuples. Then a case like ConditionSet((x, y), x<0, Integers) could raise by checking baseset.is_tuple_set is False.
|
@oscarbenjamin, the checking of the signature/base_set coherence has been removed. |
|
The tests have failed because of unused imports but this looks good to me |
References to other Issues or PRs
closes #19520
closes #19517
have not checked if #11174 has been fixed with these changes
Brief description of what is fixed or changed
Other comments
Release Notes
subs