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

Fix and test derivative(x, 2x) #1078

Conversation

LilithHafner
Copy link
Contributor

Fixes #1077

@LilithHafner LilithHafner marked this pull request as draft February 26, 2024 22:39
@LilithHafner LilithHafner marked this pull request as ready for review February 27, 2024 14:01
@LilithHafner
Copy link
Contributor Author

This breaks Symbolics.derivative(2x, 2x) == 1. IDK if we care about that.

@ChrisRackauckas
Copy link
Member

I am fine with having more errors, more safety.

@LilithHafner
Copy link
Contributor Author

This also breaks differentiating with respect to an array of symbols (which we should presumably fix) and with respect to x(t) which occurs once in tests.

src/diff.jl Outdated Show resolved Hide resolved
@LilithHafner
Copy link
Contributor Author

The crux of this issue is that we assume two expressions are constant with respect to eachother unless proven otherwise.

julia> @variables x, y

julia> Symbolics.derivative(x, y)
0

This PR does not fix that issue, and does introduce errors which previously worked e.g. all of these results will become errors:

julia> @variables x(t), y(t)

julia> Symbolics.derivative(x, x) # Good
1

julia> Symbolics.derivative(t, x) # Wrong
0

julia> Symbolics.derivative(x, t) # Good
Differential(t)(x(t))

src/diff.jl Outdated
@@ -31,8 +31,13 @@ julia> D3 = Differential(x)^3 # 3rd order differential operator
"""
struct Differential <: Operator
"""The variable or expression to differentiate with respect to."""
x
Differential(x) = new(value(x))
x::BasicSymbolic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't limit it to BasicSymbolic

src/diff.jl Outdated
x::BasicSymbolic
function Differential(x)
vx = value(x)
vx isa BasicSymbolic && SymbolicUtils.exprtype(vx) == SymbolicUtils.SYM ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just check !istree

Copy link
Member

@YingboMa YingboMa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR uses unnecessary internal details.

Differential(x) = new(value(x))
function Differential(x)
vx = value(x)
istree(vx) && throw(ArgumentError("Cannot differentiate with respect to $vx"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also not quite correct. We frequently need to differentiate wrt variables with an independent variable like x(t).

@LilithHafner LilithHafner deleted the lh/derivative-expr-error branch May 9, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

derivative gives wrong answer when differentiating with respect to an expression
3 participants