basic implementation of ConditionalExpression, Assuming and $Assumptions#1271
basic implementation of ConditionalExpression, Assuming and $Assumptions#1271
ConditionalExpression, Assuming and $Assumptions#1271Conversation
|
@mmatera I am looking forward to reviewing this in detail, but it will be maybe towards the end of the week when I can look at and understand. |
rocky
left a comment
There was a problem hiding this comment.
I spent some time trying ConditionalExpression, Assuming and $Assumptions, and aside from the words getting recognized, I couldn't make this do anything that seemed useful.
I will put in a PR into this branch for mostly small doc-change stuff.
My take is merge this when it does something useful, and for that probably just hooking this into Piecewise evaluation would do it.
2ca5e76 to
bd76d24
Compare
|
I think this is ready now: the function |
| # If the result is defined as a Piecewise expression, | ||
| # use ConditionalExpression. | ||
| # This does not work now because the form sympy returns the values | ||
| if result.get_head_name() == "System`Piecewise": |
There was a problem hiding this comment.
Another small thing. I think the idiom typically used is result.is_form() right?
There was a problem hiding this comment.
Just to have in mind, has_form is a method of Expression, so to use it, we need to check first that the object is not an atom...
There was a problem hiding this comment.
even checking with is_atom() the method fails for strings. For this reason, I reverted this to the previous strategy. Maybe later a better approach comes up.
There was a problem hiding this comment.
Here is a suggestion: add the has_form() method to Symbol. The implementation is pretty simple: return False.
There was a problem hiding this comment.
That is a great idea! I implemented it in BaseExpression instead of Symbol.
A superficial look through and I think this is exactly the right thing to do. However I want to pull this down and try it and maybe even experiment with the code to understand what's up with sympy mentioned in one of the comments. And probably the weekend is when I'll have an opportunity to do. |
75ab288 to
e5ff1ce
Compare
Good. So, I made the corrections that you mention above, and I have rebased to master. Now I leave this here until you look at this. I will continue with other issues then. |
mathics/builtin/arithmetic.py
Outdated
| "%(name)s[items__]" | ||
| result = self.to_sympy(Expression("Piecewise", *items.get_sequence())) | ||
| result = self.to_sympy( | ||
| Expression("Piecewise", *items.get_sequence()), evaluation=evaluation |
There was a problem hiding this comment.
Great! This is what I was expecting to see!
There was a problem hiding this comment.
I think this is ready now: the function
predicate_evaluation(predicate, evaluation)looks at $Assumptions and implement some replacements on predicate accordingly. This function is called then in the evaluation ofPiecewise, andConditionalExpression, in a way that propagates automatically toIntegrate.A superficial look through and I think this is exactly the right thing to do. However I want to pull this down and try it and maybe even experiment with the code to understand what's up with sympy mentioned in one of the comments. And probably the weekend is when I'll have an opportunity to do.
Update: I realized that in WMA, logic expressions are evaluated using assumptions just if you surround them by Simplify. On the other hand, Integrate uses the option Assumptions (default $Assumptions) before return the result. Now this behavior is implemented.
There was a problem hiding this comment.
Also, I found -I guess- a safe way to reproduce the use of ConditionalExpression instead of Piecewise in the return of Integrate.
rocky
left a comment
There was a problem hiding this comment.
Just had a chance to go over in detail and here's what I encountered.
Assuming[0 < n < 1, Integrate[x^n, {x, 0, 1}]]
Out[3]= Piecewise[{{1 / (1 + n) - 0 ^ (1 + n) / (1 + n), n > -Infinity && n < Infinity && n != -1}, {Infinity, True}}]
In[4]:= Assuming[n = 1, Integrate[x^n, {x, 0, 1}]]
Out[4]= 1 / 2
In[5]:= Assuming[n = 2, Integrate[x^n, {x, 0, 1}]]
Out[5]= 1 / 3
In[6]:= Assuming[Or[n = 2, n = 1] , Integrate[x^n, {x, 0, 1}]]
Out[6]= 1 / 2
In[7]:= Assuming[Or[n = 1, n = 2] , Integrate[x^n, {x, 0, 1}]]
Out[7]= 1 / 3
If you want to call this a current limitation, ok. If you'd like to explain why we get what we get, I'd appreciate it.
I leave to you whether we want to fix or merge and fix later.
In f12c348 I removed an unused import. See also #1313 if these are tweaks you want to add.
The first output is just wrong, but is wrong due an issue in sympy. The problem we have here is the format of the sympy return: we get something that is a piecewise definition, with the last (default) value being the unevaluated integral, but not exactly in the same form. So, there is no safe way to reduce this to a ConditionalExpression, without performing a more or les extense set of tests in the default value... |
And what about the last two? "Or" to me doesn't mean that you can pick which ever one you want, but that both may be true. Note that saying "And" here would be wrong since |
You are right: to assume |
Yes, that is DeMorgan's rule. Changing the single equal |
|
|
In any case, it is clear that |
|
Update: You are assigning here, so this is why it evaluates to the first number. If you use |
16f2e5d to
1426585
Compare
|
Well, this is getting bigger and bigger. I tried to delegate the predicate evaluation to sympy, but it is simply very limited, and too complicated to use here. So, I implemented our own evaluator. There are several aspects that can be improved, but I think this is a good point to merge this. |
Integrate uses now ConditionalExpression Remove an unused import Misc Arg tweaks: * Arg[0] needs to be defined 0 (otherwise it can be indeterminate) * More complete doc for Arg * Add Method option improving the behaviour of Piecewise returns in Integrate adding tests new inference module. Many Improvings on inference disabling debug prints in inference
1426585 to
d809887
Compare
rocky
left a comment
There was a problem hiding this comment.
While there is great stuff in here, I am getting concerned that the overall number of changes is getting too hard to review and understand.
It would be great to figure out how to split this up into smaller pieces and deliver those. (Sort of like I have been trying to do with adding builtin subdirectories/subcategories.
Some of the changes are trivial and not likely to change like Integer(0) -> Integer0 and Symbol("True")->SymbolTrue.
Something else easy might be Nand and Nor.
Is there anything else though?
Maybe FixedPoint with the SameTest options?
Number of commits and size of commit has grown.
rocky
left a comment
There was a problem hiding this comment.
Let's break this up. I am happy to help here.
|
If you want, most of the non-trivial changes are included in the new module |
In this PR, I addressed issue #1231, together with a skeleton for handling
Assumingand$Assumptions.ConditionalExpressionallows a methodto_sympy, which translates it to aPiecewiseexpression insympy.$Assumptionsare not currently taken into account in the evaluations, but it can be hooked in the future in the implementation ofSimplify,Reduce,Integrate, and other symbols.