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

Derivative ignores evaluate=False for nested derivative #16350

Closed
mloubout opened this issue Mar 20, 2019 · 14 comments · Fixed by #22647
Closed

Derivative ignores evaluate=False for nested derivative #16350

mloubout opened this issue Mar 20, 2019 · 14 comments · Fixed by #22647
Labels
calculus Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already.

Comments

@mloubout
Copy link
Contributor

Hello

There is a small issue in Derivative that is it is still evaluated n matter evaluate for nested derivative dues to these lines (1340-1343 in cord/functions.py:

        # denest
        if isinstance(expr, Derivative):
            variable_count = list(expr.variable_count) + variable_count
            expr = expr.expr
            return Derivative(expr, *variable_count, **kwargs)

This part is highly problematic for inheritance for two reasons:

expression is evaluated AND even if it were decided that this is necessary, the last line should be
return self.func(expr, *variable_count, **kwargs) so that inheritance doesn't break.

cheers

@asmeurer asmeurer added the Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. label Mar 22, 2019
@Mohitbalwani26
Copy link
Member

Can you please look my pull request and suggest me changes i need to make?

@Saanidhyavats
Copy link
Contributor

@mloubout I am interested to contribute on this issue, is this issue open?

Saanidhyavats added a commit to Saanidhyavats/sympy that referenced this issue Dec 29, 2019
@Saanidhyavats
Copy link
Contributor

I think we can't replace Derivative with self as the whole thing comes under new function? What else can we do to avoid the breakage of inheritance?

@mloubout
Copy link
Contributor Author

mloubout commented Jan 2, 2020

Can you do cls.__new_ instead of self.func? This would avoid the self breakage and not break inheritance as cos would be whatever inherited class it is.

@ghost
Copy link

ghost commented Feb 4, 2020

@mloubout, Could you please explain the question again?I'm a newcomer and I don't have an idea on where to get started (I found the functions.py and the line which is mentioned in issue) and I didn't understand the question. Thanks.

@Saanidhyavats
Copy link
Contributor

@mloubout I am getting the following 2 errors:
a)TypeError: object.__new__(X): X is not a type object (exp)
b)variable_count = cls._sort_variable_count(variable_count) AttributeError: 'f' object has no attribute '_sort_variable_count
can you guide me on it as I am unable to rectify the problem

@Mayank-gaur
Copy link

I am a beginner in the open source world and I want to work on this issue. Can someone please guide me how can i get started on this? (I already have sympy environment installed locally)

aradhana72 added a commit to aradhana72/sympy that referenced this issue Mar 2, 2021
added return self.func(expr, *variable_count, **kwargs) in last line so that inheritance doesn't break.
@wermos
Copy link
Contributor

wermos commented Nov 22, 2021

Hi, this is labelled as an easy to fix issue and has been open for more than 2 years now. I can see that some progress has been made here, but it hasn't been merged to the master branch or any of the release branches.

I would like to fix this and make my first contribution to SymPy. Is this issue still valid and worth fixing?

If it is, should I piggyback off of one of the existing PRs or start afresh?

@oscarbenjamin
Copy link
Collaborator

The issue is still valid and is worth fixing IMO.

If one of the existing PRs solves the problem correctly and has reasonable tests then it would be best just to identify which PR that is so that it can be merged.

Otherwise it doesn't look like the existing PRs contain substantial amounts of code changes so I don't see any need to piggyback off of them.

@wermos
Copy link
Contributor

wermos commented Nov 22, 2021

Great! I shall look into the code and do my best to solve the issue, then.

@wermos
Copy link
Contributor

wermos commented Nov 28, 2021

@oscarbenjamin While analyzing the code and the bug itself, I realized that I don't understand the issue very well.

So if we have something of the form Derivative(Derivative(expr, x), x) it's supposed to simplify it to Derivative(expr, x) and then return, right?

As far as I can tell, that is what is already happening. I would greatly appreciate an actual example that I can work with to understand the problem better.

@oscarbenjamin
Copy link
Collaborator

What currently happens is this:

In [27]: f = Function('f')

In [28]: str(Derivative(f(x), x))
Out[28]: 'Derivative(f(x), x)'

In [29]: str(Derivative(Derivative(f(x), x), x))
Out[29]: 'Derivative(f(x), (x, 2))'

That's all fine. Differentiating twice can collapse to a single second derivative.

However in some situations we don't want that collapse and it should be possible to get that behaviour when using evaluate=False:

In [30]: str(Derivative(Derivative(f(x), x), x, evaluate=False))
Out[30]: 'Derivative(f(x), (x, 2))'

This last example should ideally output:

Derivative(Derivative(f(x), x), x)

@wermos
Copy link
Contributor

wermos commented Nov 28, 2021

Ah, I get it now. Thank you for the clarification!

@mloubout
Copy link
Contributor Author

Sweet. Sorry for the silence really half to see this fixed. Thanks @faze-geek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calculus Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already.
Projects
None yet
8 participants