Skip to content

Conversation

rikardn
Copy link
Contributor

@rikardn rikardn commented Jul 28, 2021

Use some basic assumptions of positivity and negativity to refine min and max

if (is_true(is_negative(*newarg, assumptions_))) {
keep.push_back(newarg);
have_negative = true;
} else if (is_true(is_nonpositive(*newarg, assumptions_))) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be written as is_false(is_negative()) and reuse is_negative()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @isuruf for reviewing. Yes, it should be possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This turns out to be problematic. Both is_false(is_negative(expr)) and is_false(is_nonnegative(expr)) can be true in the case where expr is non-real. I can go ahead with this change, but then I would need to check is_real first.

The option to this is to let is_negative etc throw for non-real numbers. My current idea for all is_ has been to allow all numeric input and throw for Sets, Logic etc.

Copy link
Member

Choose a reason for hiding this comment

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

What does sympy do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sympy does not throw for non-reals:

>>> import sympy
>>> sympy.I.is_positive
False

So either what we have in the PR now or first check is_real then is_negative results can be inverted to get is_nonnegative for free. What do you think @isuruf?

@rikardn rikardn merged commit 562c957 into symengine:master Mar 9, 2022
@rikardn rikardn deleted the refineminmax branch March 9, 2022 09:41
@rikardn
Copy link
Contributor Author

rikardn commented Mar 9, 2022

Thanks for reviewing

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.

2 participants