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
Introduce polyadic predicate based on multipledispatch #20656
Conversation
✅ Hi, I am the SymPy bot (v161). 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.8. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
I collected some previous PRs to this one. Ping to the reviewers who participated in older versions - @oscarbenjamin @asmeurer @eric-wieser . Can you take a look at this, please? |
Ping again @oscarbenjamin , @asmeurer , @eric-wieser |
sympy/assumptions/assume.py
Outdated
|
||
@property | ||
def args(self): | ||
return self._args[1:] | ||
return self._args[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this method needed? Can we not just let the Basic.args
property return the args as normal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal way will return AppliedPredicate
for Q.even(2).func
and (Q.even, (2,))
for Q.even(2).args
.
By overriding args
like this, we can make Q.even(2).func
return Q.even
and Q.even(2).args
return (2,)
. I think this way is better since we should give different heads for different predicates, such as Q.even()
, Q.odd()
, Q.positive()
, ... etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this way is better since we should give different heads for different predicates
I don't understand what this means.
I think that almost all cases where args is overridden like this in the sympy codebase are warts to be removed. This always leads to problems down the line and it would generally be better if we could just accept the Basic model of func and args rather than fighting it.
(I realise that this wasn't added by you)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I will try fixing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fix this, but func
and args
were used everywhere in assumptions module that it exceeds the scope of this PR. I will make another PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
In general I think that if args
is different from _args
then something is wrong. At least they could be made equal and .func
could be made to work with whatever is stored in _args
.
Generally though everything is smoother if obj.func
is type(obj)
and _args
is the args that would be passed to it. This eliminates the needs for a lot of hacky workarounds.
Having the predicate not in args
means it can't be substituted e.g.:
In [1]: p = Q.even(x)
In [2]: p
Out[2]: Q.even(x)
In [3]: p.subs(Q.even, Q.odd)
Out[3]: Q.even(x)
The analogy with functions is
In [4]: sin(x).subs(sin, cos)
Out[4]: cos(x)
In [5]: f(x).subs(f, g)
Out[5]: g(x)
This is made to work in a hacky way because sin
, cos
, f
and g
are not Basic
. If they were Basic
and f(x)
was just Call(Function('f'), Symbol('x'))
then subs would work automatically. That's what what we want to aim for in Function and it's one of the chief advantages of having the function objects themselves be Basic
. Disallowing that in the case of Predicate
seems backward to me.
At the same time code should not make any presumptions about what obj.func
is except that it can be called with the args as obj.func(*obj.args)
so if obj.func
is being used for anything else then that should be changed as well.
I left some comments but broadly I think this looks good. Should the docs recommend using |
@asmeurer @eric-wieser do you want to review this? |
I designed the |
@oscarbenjamin I introduced |
It would be better if we could avoid using a metaclass but otherwise I think this looks good. @asmeurer @eric-wieser do you want to review? |
Squashed the commits |
@oscarbenjamin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some grammar fixes and documentation suggestions
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Thanks for the review! |
Okay this looks good. Thanks and sorry for the long wait on this! |
Does the new predicate system here fix this problem #20635 (comment)? |
References to other Issues or PRs
Closes #20209
Brief description of what is fixed or changed
This PR improves the predicate and handler design in
assumptions
module.Predicate("...")
now returnsUndefinedPredicate
, which cannot be evaluated. To make predicate that can be evaluated, subclass ofPredicate
must be defined.Predicate
can now take multiple arguments.This PR does not break back compatibility. Existing handlers are not yet migrated to this new design. It will be done in succeeding PRs.
Also, printing of predicates is improved and tests are added.
Other comments
Previous handler design was very unstable. It made
Predicate
mutable, it filters the argument by its class "name", and could not take multiple arguments. This new multipledispatch design fixes these.Just as
Function('f')
returns undefined function which cannot be evaluated,Predicate('...')
returnsUndefinedPredicate
in this PR. This instance does not have handler and cannot be evaluated by its own, but it can be used to construct expressions likeNot
and evaluated by SAT solver.To define a new predicate with handler, new predicate class must be defined and functions must be dispatched to its handler. For example,
Evaluating this to boolean is done by
ask
as usual.Release Notes
Predicate
now uses a single handler which is multipledispatch instance.Predicate
can now take multiple arguments.Predicate("...")
now returnsUndefinedPredicate
instance. To define a predicate, you must make a subclass ofPredicate
.