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

Add Reals set #1686

Merged
merged 1 commit into from Sep 19, 2020
Merged

Add Reals set #1686

merged 1 commit into from Sep 19, 2020

Conversation

rikardn
Copy link
Contributor

@rikardn rikardn commented Sep 16, 2020

Implementation of the set of real numbers: Reals.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this PR. Got a couple of questions

@@ -233,6 +233,46 @@ vec_basic Interval::get_args() const
return {start_, end_, boolean(left_open_), boolean(right_open_)};
}

RCP<const Set> Reals::set_intersection(const RCP<const Set> &o) const
{
return o;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o could have complex values right?

Copy link
Contributor Author

@rikardn rikardn Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I'll try to figure out how to do this. Ideas on how to handle complex numbers are welcome. I am thinking:

Interval, Empty - Must be real and can be handled as currently
FiniteSet - Can contain complex numbers. Could be handled directly
all others - Can contain complex numbers so create Intersection object


RCP<const Set> Reals::set_union(const RCP<const Set> &o) const
{
return reals();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@certik
Copy link
Contributor

certik commented Sep 16, 2020

Thanks! There is a failure in https://travis-ci.org/github/symengine/symengine/jobs/727822895:

/home/travis/build/symengine/symengine/symengine/tests/printing/test_printing.cpp:692: FAILED:

  CHECK( latex(*l13) == "5 \\leq b \\wedge 2 \\leq a" )

with expansion:

  "2 \leq a \wedge 5 \leq b"

  ==

  "5 \leq b \wedge 2 \leq a"

/home/travis/build/symengine/symengine/symengine/tests/printing/test_printing.cpp:694: FAILED:

  CHECK( latex(*l14) == "b \\leq a \\wedge \\left(a \\neq c \\vee a = b\\right)" )

with expansion:

  "b \leq a \wedge \left(a = b \vee a \neq c\right)"

  ==

  "b \leq a \wedge \left(a \neq c \vee a = b\right)"

Not sure if it is related to this PR?

@rikardn
Copy link
Contributor Author

rikardn commented Sep 16, 2020

The test failure seems to be related to this PR. I reverted and ran the tests without fail. I will try to resolve it.

@rikardn
Copy link
Contributor Author

rikardn commented Sep 16, 2020

I see that I also missed adding Reals to is_a_Set. I'll do that.

@isuruf-bot
Copy link

isuruf-bot commented Sep 17, 2020

Hi,

I've run clang-format and found that the code formatting is good.

Thanks for fixing the formatting.

@rikardn
Copy link
Contributor Author

rikardn commented Sep 17, 2020

I have

  • added Reals to is_a_Set
  • Fixed the set_union and set_intersection to handle complex numbers
  • Fixed the failing test. At least for now. It seems as if the failing was due to reversed ordering of a set.

@rikardn
Copy link
Contributor Author

rikardn commented Sep 17, 2020

I see the code formatting issue. Is this fixed automatically or should I fix manually?

@certik
Copy link
Contributor

certik commented Sep 17, 2020

You have to manually apply the commands from the message.

if (is_a<Reals>(*o)) {
set_basic container;
for (const auto &elem : container_) {
if (is_a<Complex>(*elem)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking if the value is a complex number here, we should check if the value is not a real number. For eg: symbols should be considered complex since we don't know what the value is going to be.

@rikardn
Copy link
Contributor Author

rikardn commented Sep 18, 2020

Thanks for your patience! I have updated again:

  • Fixed the FiniteSet union and intersection as suggested
  • Added a proper contains method which was forgotten

The intersection doesn't fully work because of missing functionality in the set_intersection function. I don't know if you intend to have a separate class for intersections or if you want to solve it with unions and complements via deMorgan. In any case I don't know if anything else must be done about that in this PR.

@rikardn
Copy link
Contributor Author

rikardn commented Sep 19, 2020

Now also the code coverage check passes. Are we good to go?

@isuruf isuruf merged commit 8fd3313 into symengine:master Sep 19, 2020
@isuruf
Copy link
Member

isuruf commented Sep 19, 2020

Thanks for the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants