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

Added is_Boolean = True to relationals #24087

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MauriceHendrix
Copy link

@MauriceHendrix MauriceHendrix commented Sep 26, 2022

References to other Issues or PRs

Fixes #24086

Brief description of what is fixed or changed

Added is_Boolean = True to reationals to fix issue where booleans in equations being passed to Piecewise cause error AttributeError: 'BooleanTrue' object has no attribute 'as_coeff_Add' as suggested by @oscarbenjamin

Other comments

  • I tested the issue with sympy 1.9, 1.10.1 and 1.11.1 it does not happen in 1.9 but does in 1.10.1 and 1.11.1
  • We are working on a project getting equations form MathML in an xml file (cellml). https://github.com/ModellingWebLab/cellmlmanip

Release Notes

  • core
    • Fixed a bug in Relational where booleans in equations being passed to Piecewise cause error.

@sympy-bot
Copy link

sympy-bot commented Sep 26, 2022

Hi, I am the SymPy bot (v167). 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:

  • core
    • Fixed a bug in Relational where booleans in equations being passed to Piecewise cause error. (#24087 by @MauriceHendrix)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12.

Click here to see the pull request description that was parsed.
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
Fixes #24086


#### Brief description of what is fixed or changed
Added is_Boolean = True to reationals to fix issue where booleans in equations being passed to Piecewise cause error `AttributeError: 'BooleanTrue' object has no attribute 'as_coeff_Add' ` as suggested by @oscarbenjamin


#### Other comments
- I tested the issue with sympy 1.9, 1.10.1 and 1.11.1 it does not happen in 1.9 but does in 1.10.1 and 1.11.1
- We are working on a project getting equations form MathML in an xml file (cellml). https://github.com/ModellingWebLab/cellmlmanip


#### Release Notes
<!-- BEGIN RELEASE NOTES -->
* core
  * Fixed a bug in Relational where booleans in equations being passed to Piecewise cause error.
<!-- END RELEASE NOTES -->

@jksuom
Copy link
Member

jksuom commented Sep 26, 2022

Shouldn't this be in class Boolean?

@oscarbenjamin
Copy link
Contributor

Shouldn't this be in class Boolean?

The problem is that Symbol inherits from Boolean and therefore occupies an ambiguous state where it is both Expr and Boolean at the same time:

In [3]: Symbol.mro()
Out[3]: 
[sympy.core.symbol.Symbol,
 sympy.core.expr.AtomicExpr,
 sympy.core.basic.Atom,
 sympy.core.expr.Expr,
 sympy.logic.boolalg.Boolean,
 sympy.core.basic.Basic,
 sympy.printing.defaults.Printable,
 sympy.core.evalf.EvalfMixin,
 object]

The purpose of the is_Boolean check here is to distinguish Expr from Boolean:

if not rel.is_Relational or rel.rhs.is_Boolean:
return rel # Eq(x, True)
b, l = rel.lhs.as_coeff_Add(rational=True)

As you can see from the comment Eq(x, True) it is possible that we would have a Symbol (both Expr and Boolean) on one side and a proper Boolean on the other. We could equally have Eq(x, 0) where the rhs is a proper Expr.

I don't know if we have an issue for this but there should really be a BooleanSymbol that is separate from Symbol and there should not be any objects that are both Expr and Boolean because that's clearly a contradiction.

@asmeurer
Copy link
Member

Symbol should be fixed to use the kind system.

But also I don't think Eq should allow boolean arguments like True. There's code in Eq that assumes the arguments are Expr. The boolean version of equality is Equivalent.

@oscarbenjamin
Copy link
Contributor

Symbol should be fixed to use the kind system.

It does use the kind system:

In [151]: Symbol('x').kind
Out[151]: NumberKind

The problem is that this is inconsistent with its usage as a boolean and there is no BooleanSymbol to be used with kind BooleanKind.

@asmeurer
Copy link
Member

Isn't the point of the kind system that we don't need a bunch of different Symbol classes to represent different types of symbols? There can be just one Symbol and it can have NumberKind or BooleanKind or whatever depending on what sort of object it represents.

@oscarbenjamin
Copy link
Contributor

Isn't the point of the kind system that we don't need a bunch of different Symbol classes to represent different types of symbols? There can be just one Symbol and it can have NumberKind or BooleanKind or whatever depending on what sort of object it represents.

Yes, that's the idea. It should be possible to just do symbols('x, y', kind=BooleanKind) or there could be a more convenient shorthand for that. Currently that isn't possible though and for now Symbol is a subclass of both Expr and Boolean. In extensive discussions when introducing kinds I made it clear that they should be mutually exclusive so there should be no possibility for an object to be e.g. possibly a Boolean or possibly a Number. The result of that is that Symbol.kind is NumberKind but we need an alternative for the boolean case. There needs to be a way for the user to say that the symbol is supposed to represent a boolean value rather than a number but we don't have that.

@oscarbenjamin oscarbenjamin changed the title Added is_Boolean = True to reationals Added is_Boolean = True to relationals Nov 14, 2022
@oscarbenjamin
Copy link
Contributor

The authors test failed because the mailmap file should be sorted. Running the mailmap_check.py script will do this.

There are also some failures in stats:

__________________________ sympy.stats.rv.independent __________________________
File "/home/runner/work/sympy/sympy/sympy/stats/rv.py", line 1533, in sympy.stats.rv.independent
Failed example:
    independent(X, Y)
Expected:
    False
Got:
    True
________________________________________________________________________________
___________________________ sympy.stats.rv.dependent ___________________________
File "/home/runner/work/sympy/sympy/sympy/stats/rv.py", line 1496, in sympy.stats.rv.dependent
Failed example:
    dependent(X, Y)
Expected:
    True
Got:
    False
________________________________________________________________________________
______________________ sympy.stats.rv_interface.kurtosis _______________________
File "/home/runner/work/sympy/sympy/sympy/stats/rv_interface.py", line 335, in sympy.stats.rv_interface.kurtosis
Failed example:
    kurtosis(X, X > 0) # find kurtosis given X > 0
Expected:
    (-4/pi - 12/pi**2 + 3)/(1 - 2/pi)**2
Got:
    3
________________________________________________________________________________
______________________ sympy.stats.rv_interface.skewness _______________________
File "/home/runner/work/sympy/sympy/sympy/stats/rv_interface.py", line 297, in sympy.stats.rv_interface.skewness
Failed example:
    skewness(X, X > 0) # find skewness given X > 0
Expected:
    (-sqrt(2)/sqrt(pi) + 4*sqrt(2)/pi**(3/2))/(1 - 2/pi)**(3/2)
Got:
    0

I'm not sure what has caused those failures.

@oscarbenjamin oscarbenjamin marked this pull request as draft November 14, 2022 20:48
@oscarbenjamin
Copy link
Contributor

I'm marking this as draft. Feel free to mark it as not draft if it becomes ready for review again. The main thing needed is to figure out what exactly has caused the stats failures.

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