Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConditionSet #1291

Merged
merged 10 commits into from Jun 20, 2017

Conversation

Projects
None yet
6 participants
@ranjithkumar007
Copy link
Contributor

commented Jun 8, 2017

ping @nishnik.

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from c997889 to f911edb Jun 9, 2017

@@ -94,7 +94,7 @@ RCP<const Set> Interval::close() const

RCP<const Boolean> Interval::contains(const RCP<const Basic> &a) const
{
if (not is_a_Number(*a))
if (not is_a_Number(*a) and not is_a<Constant>(*a))

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 9, 2017

Member

Does this work?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 9, 2017

Author Contributor

no. I shall remove this.

{
private:
vec_sym syms_;
RCP<const Set> base_;

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 9, 2017

Member

Why can't this be in condition?

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from 00fc64b to e78b79c Jun 9, 2017

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2017

@isuruf ,
Should And(a,b) evaluate to contains(x,[1,2]) where a=contains(x,[1,5]) and b=contains(x,[1,2]) or remain as unevaluated?

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from e78b79c to 64530e1 Jun 10, 2017

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2017

ping @isuruf

@isuruf-bot

This comment has been minimized.

Copy link

commented Jun 11, 2017

Hi,

I've run clang-format and found that the code needs formatting.
Here's a commit that fixes this. isuruf-bot@ab97e0f

To use the commit you can do

curl -o format.diff https://github.com/isuruf-bot/symengine/commit/ab97e0f9c1eafa20a0ef418b65831a5cc104ded6.diff
git apply format.diff

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from 6426a47 to 322eab5 Jun 11, 2017

@Sumith1896

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

Ah this is interesting :D

@@ -65,6 +65,8 @@ class Contains : public Boolean
RCP<const Set> get_set() const;
virtual vec_basic get_args() const;
virtual bool __eq__(const Basic &o) const;
virtual RCP<const Basic> create(const RCP<const Basic> &lhs,

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 12, 2017

Member

Is virtual needed?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 12, 2017

Author Contributor

ah. nope
Thanks

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from 322eab5 to d4198b7 Jun 13, 2017

@ranjithkumar007 ranjithkumar007 referenced this pull request Jun 13, 2017

Merged

Imageset #1293

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2017

@@ -137,7 +137,7 @@ TEST_CASE("And, Or : Basic", "[basic]")
auto int1 = interval(integer(1), integer(2), false, false);
auto int2 = interval(integer(1), integer(5), false, false);
auto c1 = contains(x, int1);
auto c2 = contains(x, int2);
auto c2 = contains(symbol("y"), int2);

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

why this change?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 14, 2017

Author Contributor

I will revert this. I earlier changed this because initially I simplified contains.

@@ -237,7 +237,7 @@ TEST_CASE("Not : Basic", "[basic]")
auto int1 = interval(integer(1), integer(2), false, false);
auto int2 = interval(integer(1), integer(5), false, false);
auto c1 = contains(x, int1);
auto c2 = contains(x, int2);
auto c2 = contains(symbol("y"), int2);

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

Why this change?

const FiniteSet &fs = down_cast<const FiniteSet &>(*o);
auto container = fs.get_container();
if (syms_.size() != container.size()) {
throw std::runtime_error("size of symbols didn't match");

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

Why does this have to match?

class ConditionSet : public Set
{
private:
vec_sym syms_;

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

Why is this a vector? SymPy only has one symbol?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 14, 2017

Author Contributor
>>> ConditionSet({x,y},x+y>4,S.Reals)
{{x, y} | {x, y} ∊ ℝ ∧ x + y > 4}

SymPy supports multiple symbols.

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

Is this intended or not? I don't see any documentation with multiple symbols.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 14, 2017

Author Contributor

https://github.com/sympy/sympy/blob/master/sympy/solvers/tests/test_solveset.py#L1383
This is the only point I could find where SymPy actually uses multiple symbols for ConditionSet.
Should we make it only for a single variable then?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

Yes, you can make it a single variable and you can use a FiniteSet (which is a single entity) if there is a need

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 14, 2017

Member

Yes. RCP<const Basic> is good enough.

@isuruf-bot

This comment has been minimized.

Copy link

commented Jun 14, 2017

Hi,

I've run clang-format and found that the code needs formatting.
Here's a commit that fixes this. isuruf-bot@af08530

To use the commit you can do

curl -o format.diff https://github.com/isuruf-bot/symengine/commit/af085308e1babb2a2ed8b9d7648d13d0c688d1af.diff
git apply format.diff

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from 5585a4b to 39e2a0e Jun 14, 2017

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2017

thanks @isuruf-bot :D

SYMENGINE_ASSERT(ConditionSet::is_canonical(sym, condition))
}

bool ConditionSet::is_canonical(const RCP<const Basic> sym,

This comment has been minimized.

Copy link
@srajangarg

srajangarg Jun 15, 2017

Contributor

If condition does not involve symbol sym at all, it should be the UniversalSet and return false?

This comment has been minimized.

Copy link
@srajangarg

srajangarg Jun 15, 2017

Contributor

What I mean is something like {x | y > 0}

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 15, 2017

Author Contributor

if we return UniversalSet, then it would guarantee that for any x, condition y>0 is true, which might not be true.

This comment has been minimized.

Copy link
@srajangarg

srajangarg Jun 16, 2017

Contributor

I'm not versed with the theory involved, it should either be Universal or Empty. @nishnik ?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 16, 2017

Author Contributor

ping @nishnik

This comment has been minimized.

Copy link
@nishnik

nishnik Jun 16, 2017

Contributor

I think it should remain un-evaluated as SymPy does.
Because if in any case, x gets dependent on y, we can't be sure that it is universal set or empty set (it can be an interval as well, in your example put y=x+5)
And if we consider that x and y are always going to be independent, which can easily be discarded by using three inter-dependent variables in reasoning.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 16, 2017

Author Contributor

Agreed @nishnik.

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from 39e2a0e to 15ba25d Jun 16, 2017

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2017

can you have a look @srajangarg @nishnik ?

@srajangarg

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2017

LGTM @nishnik any comments? or should I merge?

@srajangarg srajangarg closed this Jun 17, 2017

@srajangarg srajangarg reopened this Jun 17, 2017

void bvisit(const Contains &x)
{
RCP<const Basic> a = apply(x.get_expr());
RCP<const Set> b = rcp_static_cast<const Set>(apply(x.get_set()));

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

How can you be sure that the result is a Set

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

x.get_set() always returns the object of type Set, then doesn't that guarantee that output is a Set?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

@isuruf , is there any better way of doing this ?

{
set_boolean v;
for (const auto &elem : x.get_container()) {
v.insert(rcp_static_cast<const Boolean>(apply(elem)));

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

Same here

@@ -480,7 +493,7 @@ TEST_CASE("Complement : Basic", "[basic]")
REQUIRE(eq(*r4.get_universe(), *i1));

REQUIRE(r1->get_args().size() == 2);
REQUIRE(eq(*r1->contains(one), *boolTrue));
REQUIRE(is_a<Not>(*r1->contains(one)));

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

Why this change?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

at this point, r1 stores (-oo, oo) \ {y} . But here we are not sure if finiteset({y}) contains one or not.
So, this should return not(contains(1,{y})). Earlier this bug was overshadowed by the bug in FintieSet contains, which I corrected now.

r1 = r2->set_intersection(finiteset({zero, integer(2)}));
REQUIRE(eq(*r1, *finiteset({zero, integer(2)})));
CHECK_THROWS_AS(r2->set_intersection(finiteset({zero, integer(2)})),
std::runtime_error);

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

Why this change?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

similar argument as ^above here.
r2 contains (-oo, oo) \ {y}. so we can't simplify this unless we have info of y. So, this should return an object of type Intersection class which is currently not implemented. So, runtime_error.

RCP<const Set> conditionset(const RCP<const Basic> &sym,
const RCP<const Boolean> &condition)
{
if (ConditionSet::is_canonical(sym, condition)) {

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

Why this? Everything is checked twice because of this

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

Thanks. Will be updated.

if (is_a<Contains>(**it)
and eq(*down_cast<const Contains &>(**it).get_expr(), *sym)
and is_a<FiniteSet>(
*down_cast<const Contains &>(**it).get_set())) {

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

Can you comment in the code explaining what you are doing here?

{
return contains(lhs, rhs);
return contains(lhs, rcp_static_cast<const Set>(rhs));

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

Why? You should never use rcp_static_cast if you are unsure of the type

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

then how should this be done?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

I mean how can I call contains whose second param is a Set?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

You have to check that the object is a Set and throw an error if not.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

Okay ! Thanks

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

this check is to be done here in create or in subs ?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

No, in subs.

@@ -215,9 +215,13 @@ const set_boolean &And::get_container() const
return container_;
}

RCP<const Basic> And::create(const set_boolean &a) const
RCP<const Basic> And::create(const vec_basic &a) const

This comment has been minimized.

Copy link
@isuruf
*down_cast<const Contains &>(**it).get_set())) {
// iterate through container and check for the condition that
// defines the domain of sym.
// Simplify if that set is a FiniteSet.

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 17, 2017

Member

These types of simplification logic does not belong here. If And can be simplified to a Contains with a FiniteSet, then that should happen in And.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 17, 2017

Author Contributor

Got it.

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2017

ping @isuruf

return SymEngine::set_union(
{SymEngine::set_union(temp),
conditionset(sym, logical_and(newcont))});
}

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

Can you explain what's the purpose of the lines 923-948?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

@isuruf , I will try to explain this with an example.
say logical_and({0,1,-3,y}->contains(x),x>0) is the given condition.
Then this gets simplified to And({1}->contains(x),{y}->contains(x),x>0) within logical_and().
Now, here we need to extract this {1} from the And to return the output of conditionset()as {1}UConditionSet(x,logical_and({y}->contains(x),x>0)). Basically, it seperates out all such finitesets which satisy the given condition for sure.

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

logical_and({0,1,-3,y}->contains(x),x>0) is not equivalent to And({1}->contains(x),{y}->contains(x),x>0)

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

why ?can't we ignore other terms in the Finite set as they don't satisfy x>0 condition?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

logical_and({0,1,-3,y}->contains(x),x>0) is equivalent to And(Or({1}->contains(x),{y}->contains(x)),x>0)

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

Also, don't do simplifications you are not sure are correct. It's better the result be in non-simplified form rather than an incorrect result.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

Okay! Thanks !!

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

SymPy doesn't do the simplification either, ConditionSet(x, And(FiniteSet(-1, 0, 2).contains(x), x > 0), S.Reals)

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

@isuruf, actually SymPy does this simplification. when we merged the base set into this condition, complicacy increased.

>>> ConditionSet(x,x>0,FiniteSet(0,1,-3,y))
{1} ∪ {x | x ∊ {y} ∧ x > 0}

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

Okay, then let's keep this

return boolean(true);
}
d[sym] = o;
return boolean(eq(*condition_->subs(d), *boolTrue));

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

This is wrong too. ConditionSet(x, sin(x)<y, S.Reals).contains(5)

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

I think it should return condition_->subs(d).

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

rcp_static_cast(condition_->subs(d)), after checking if this is of type Boolean.

for (const auto &c : cont) {
d[sym] = c;
if (not(eq(*condition_->subs(d), *boolTrue)))
return boolean(false);

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

Here you are checking the ConditionSet contains all of the elements of o. This is not what contains means. Contains means you check that the element o is contained in the set. o can be a element only if sym is a FiniteSet and this case is already handled in Line 726 and below

@isuruf-bot

This comment has been minimized.

Copy link

commented Jun 18, 2017

Hi,

I've run clang-format and found that the code needs formatting.
Here's a commit that fixes this. isuruf-bot@2fd191f

To use the commit you can do

curl -o format.diff https://github.com/isuruf-bot/symengine/commit/2fd191f8173086fefb9c3e571bdf986340ad090b.diff
git apply format.diff

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from 657370c to d94a996 Jun 18, 2017

@@ -690,16 +698,22 @@ TEST_CASE("ConditionSet : Basic", "[basic]")
REQUIRE(eq(*r1->contains(integer(3)), *boolTrue));
REQUIRE(eq(*r1->contains(integer(-3)), *boolFalse));
REQUIRE(eq(*r1->contains(integer(2)), *boolFalse));
REQUIRE(eq(*r1->contains(finiteset({integer(3)})), *boolTrue));
REQUIRE(eq(*r1->contains(finiteset({integer(3)})), *boolFalse));

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

@isuruf, this test case is failing as And(9 == {3}**2, Contains({3}, (1, oo))) doesn't evaluate to boolFalse.
Is this a problem with Interval:: contains()?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

@nishnik might be more familiar.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

I think that the problem is with Interval:: contains

diff --git a/symengine/logic.cpp b/symengine/logic.cpp
index 979ff74..24820e2 100644
--- a/symengine/logic.cpp
+++ b/symengine/logic.cpp
 RCP<const Boolean> contains(const RCP<const Basic> &expr,
                             const RCP<const Set> &set)
 {
-    if (is_a_Number(*expr)) {
-        return set->contains(expr);
-    } else {
-        return make_rcp<Contains>(expr, set);
-    }
+    return set->contains(expr);
 }
 
diff --git a/symengine/sets.cpp b/symengine/sets.cpp
--- a/symengine/sets.cpp
+++ b/symengine/sets.cpp
 
 RCP<const Boolean> Interval::contains(const RCP<const Basic> &a) const
 {
-    if (not is_a_Number(*a))
-        return make_rcp<Contains>(a, rcp_from_this_cast<const Set>());
+    if (not is_a_Number(*a)) {
+        if (is_a<Symbol>(*a) or is_a<Constant>(*a))
+            return make_rcp<Contains>(a, rcp_from_this_cast<const Set>());
+        return boolean(false);
+    }

^Above patch should fix this problem. thoughts @isuruf and @nishnik?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

With that patch interval(1, 2)->contains(y**2) returns false.

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

how to make Contains({3}, (1, oo)) return false then or shall we leave it without any further simplification?

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 19, 2017

Member

You can do as sympy does and return false for any Sets

if (not present.empty()) {
newcont.insert(finiteset(others)->contains(sym));
return SymEngine::set_union(
{finiteset(present), conditionset(sym, logical_and(newcont))});

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

What's the result of And(Contains(x, {1, 2, 3}), Eq(y, x)) and conditionset(x, And(Contains(x, {1, 2, 3}), Eq(y, x)))?

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 18, 2017

Author Contributor

And(x == y, Contains(x, {1, 2, 3})) and {1, 2, 3} respectively.

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 18, 2017

Member

Okay, we need to fix the the second result then.

@@ -935,7 +925,27 @@ RCP<const Set> conditionset(const RCP<const Basic> &sym,
if (not(is_a_Number(*elem) or is_a<Constant>(*elem))) {
others.insert(elem);
} else {
present.insert(elem);
// logical_and() doesn't guarantee that if element of a

This comment has been minimized.

Copy link
@ranjithkumar007

ranjithkumar007 Jun 19, 2017

Author Contributor

@isuruf , can you have a look here?
I added a test you suggested.

d[sym] = o;
auto cond = condition_->subs(d);
if (not is_a_Boolean(*cond)) {
throw std::runtime_error("expected an object of type Boolean");

This comment has been minimized.

Copy link
@isuruf

isuruf Jun 20, 2017

Member

Throw SymEngineException. Don't use std::runtime_error

@ranjithkumar007 ranjithkumar007 force-pushed the ranjithkumar007:conditionset branch from b4a5ad6 to 3a18dc2 Jun 20, 2017

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2017

updated @isuruf

@isuruf

isuruf approved these changes Jun 20, 2017

Copy link
Member

left a comment

Thanks for your work and patience. Let's get this merged.

@ranjithkumar007

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2017

Thanks a lot for the thorough review. :)

@isuruf isuruf merged commit 67bfa33 into symengine:master Jun 20, 2017

2 of 3 checks passed

codecov/project 82.32% (-0.02%) compared to 8b4045f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ranjithkumar007 ranjithkumar007 deleted the ranjithkumar007:conditionset branch Jun 20, 2017

isuruf added a commit to isuruf/symengine that referenced this pull request Aug 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.