-
-
Notifications
You must be signed in to change notification settings - Fork 601
Implicit derivative #12922
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
Comments
Attachment: 16191.patch.gz |
comment:3
Reviewed, seems good. Added a check for having no y term in the expression. Was previously an uncaught |
comment:4
Yes, both helpful! A few comments.
I'm also wondering - is it possible to have an implicit derivative method for symbolic expressions? Just curious. |
adds error handler |
comment:5
Attachment: 16191_review.patch.gz All the variables defined in the function are local. They do not get mixed with the global variables with the same name. You can even call the function with xx and yy as parameters, and they will not be mixed with the local xx and yy.
About writing this as a method for symbolic expressions... that was my first idea; but i prefeared this aproach, due to the fact that you are actually not diferentiating the symbolic expression, but a function defined implicitely by the symbolic expression. So it seemed more natural to me this kind of syntax. |
comment:6
Calling
|
comment:7
I have tried to use SR.symbol() instead of SR.symbol('x'), but then the result is a mess. I can't think of a way to make this work without the said side effect. |
comment:8
I solved the pynac symbol registry pollution by saving a copy of it at the beginning of the function, and restoring it at the end. I am not sure if this is the best choice, but it is the only solution i found, and seems to work fine. |
comment:9
Messing with the symbol registry this way is really not a good idea. That is just supposed to be internal data. What if there is an exception in that code and the registry is not restored? What exactly is the problem with using temporary variables? Looking briefly at the patch, it looks like we also need temporary functions. |
comment:10
The problem with using temporary variables without giving them a name string is this:
I don't know why, this problem is not present if we put the strings in the definitions of the variables. |
comment:11
Replying to @miguelmarco:
It looks like the substitution doesn't work at some point. Looking at the code... The calls BTW, the |
comment:12
Before this gets switched to positive review, we should check how it behaves if there already is a symbolic function defined with a custom method for evaluation named Why is this not a method of symbolic expressions? Isn't it inconsistent to have the functional notation supported but not a class method? |
comment:13
Thanks for the comments. I will try to work on it. As i said, the reason why it is not a method is that it is not clear to me what class should it be in. Is this a method of the expression F? Well, not exactly, since F is not the function that we are trying to differentiate, but the symbollic expression that deffines it implicitly. Of X? This is just the variable with which we differentiate on. Of Y? Riguorilously that should be the function that we are differentiating. But doesn't sound natural at all to add a method like this for symbollic variables. Or maybe we could define a new class ImplicitelyDefinedSymbolicFunction for this case, bat that sounds way overkill. |
Attachment: 12922.patch.gz Newer version, solves the pynac registry pollution without manipulating it |
comment:14
Ok, your suggestions seemed to work fine. I have uploaded a newer version. Please check it. |
New commits:
|
comment:25
For starters, I have the following comments:
Honestly, I have not looked at the math. I will get back about the math and the programming part in a bit. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:27
Replying to @miguelmarco:
I think Burcin wants this method part of the symbolic expression or function class. If it's not tied to a specific expression then make it a |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:29
Moved it to a method in the expression class. |
Changed branch from u/mmarco/ticket/12922 to u/rws/ticket/12922 |
Changed reviewer from Kannappan Sampath to Kannappan Sampath, Ralf Stephan |
comment:32
Thanks for the review. |
comment:33
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:35
Interesting. That doctest depends on the last line in |
Changed branch from u/rws/ticket/12922 to |
Added implicit_derivative function. It allows to compute the n'th derivative of the implicit function defined by a symbolic expression.
CC: @kcrisman @sagetrac-sjg10 @burcin
Component: calculus
Keywords: implicit derivative
Author: Miguel Marco
Branch/Commit:
0a76da7
Reviewer: Kannappan Sampath, Ralf Stephan
Issue created by migration from https://trac.sagemath.org/ticket/12922
The text was updated successfully, but these errors were encountered: