-
Notifications
You must be signed in to change notification settings - Fork 42
Speed up diff; extend interface #279
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
Conversation
There are two things here: one extends the interface, the other closes #225. The latter adds a check for whether the expression is zero. As such, it may be a performance concern. I didn't benchmark, But if there is a concern, I can modify this PR. |
This last commit to this PR has a noticeable speed up for basic differentation: Before
After
This is gained by avoiding the use of |
Hi @isuruf any objections to my merging this? |
Hi @isuruf, can I merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One question. Wouldn't it be better to have an inplace diff and call it multiple times to reduce the number of allocations?
Great idea! (I always forget Basic is mutable). I'll try and investigate soon |
end | ||
|
||
function diff(b1::SymbolicType, b2::SymbolicType, n::Integer, xs...) | ||
diff(diff(b1,b2,n), xs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use a mutating diff as well, but not necessary for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment
This implements suggestion in #225 to have a step for early termination in diff if possible.This passes on #225; the additional allocations to check for early termination would be paid in every instance; whereas the suggestion of #225 only pays off in certain situations. (The allocation could be avoided by some call to
basic_has_symbol
, but even that had a performance cost)BasicType
to check thatx
is a symbol; as this is more performantdiff(ex)
method for case whereex
has just 0 or 1 symboldiff(ex, x, 2, y)
.diff(ex, (x,2))
interface of SymPy, as that method is used otherwise. (would fix were it not breaking)repr