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
Allow more general evaluation of FDerivativeOperator #12796
Comments
comment:1
With the attached patch we have:
but unfortunately:
The problem here is in
is valid, but is currently not accepted by the string-based parser. |
Author: Nils Bruin |
comment:2
The problem of lists as arguments can be solved by changing the grammar that Another possibly controversial point is the name of the temporary variables used: now it's |
This comment has been minimized.
This comment has been minimized.
comment:6
Quite closely related: #7401, which takes a similar approach but only implements the univariate case. |
comment:7
Hmm, does that mean that the doctest in #7401 that caused it to never be merged will now not fail? That would be nice; it was really a shame that was never brought in. |
comment:8
It'll take me a while to understand the patch, but combined with #12801 and the use of
I have a lot of code I can test it against. |
comment:9
The approach looks correct to me, I've just added a bunch of doctests and fixed some small issues:
I get two deprecation warnings from my spline code, but one of them occurs on the lines that #12801 modifies, so we can fix them there. If the review patch is OK, the original is for me. |
comment:10
Replying to @orlitzky:
Thanks! that was a typo that crept in.
A nice idea, but have you tested what your solution does with "val = a = 1" ? I think the result will be (val,(a,1)), which is probably not what is intended (it should be a syntax error). I did think about a One way to avoid that is to let p_arg parse
Don't import Thanks for your work. |
comment:11
Replying to @nbruin:
Indeed. I'll revert this and remove its doctest. It used to return
We're doing a lazy_import of maxima_lib in calculus, though, or is there still a reason to avoid it? I'll just revert that too if so. |
comment:12
Replying to @orlitzky:
I know I had trouble with it before, with dummy_integral. You're right that the lazy import might have fixed the issue there. My main argument against sharing the code would be to keep the logic simple. The routines |
Revert two chunks of the review patch per comments |
comment:13
Attachment: sage-trac_12796-review.patch.gz No problem, I'm just trying to wrap my head around all of the symbolics code. I've reverted those two changes in the last patch. |
This comment has been minimized.
This comment has been minimized.
Reviewer: Michael Orlitzky |
comment:15
OK! I'm happy with the reviewer patch. Thank you for writing such comprehensive documentation. I'll leave it to you to set the ticket to positive review if you're happy now as well. |
comment:17
Why was there no newline after the commit message?... fixed now. |
Apply this patch |
comment:18
Attachment: 12796.patch.gz |
Merged: sage-5.0.beta14 |
Currently:
This can change.
Apply attachment: 12796.patch, attachment: sage-trac_12796-review.patch.
CC: @kcrisman @orlitzky
Component: symbolics
Author: Nils Bruin
Reviewer: Michael Orlitzky
Merged: sage-5.0.beta14
Issue created by migration from https://trac.sagemath.org/ticket/12796
The text was updated successfully, but these errors were encountered: