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

Diferentiate, integrate and evaluate with symbols #155

Merged
merged 5 commits into from
Feb 23, 2018

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Feb 22, 2018

This fixes #148 and fixes #151

EDIT: Correct spelling

@lbenet
Copy link
Member Author

lbenet commented Feb 22, 2018

I'm not sure if a rebase is needed here...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.005% when pulling e6d5fd3 on lb/integrate_symbols into 1c32f7c on master.

@coveralls
Copy link

coveralls commented Feb 22, 2018

Coverage Status

Coverage increased (+0.03%) to 97.155% when pulling 5b3a3f4 on lb/integrate_symbols into 1c32f7c on master.

@dpsanders
Copy link
Contributor

Yes I think it needs to be rebased. Should be trivial

@lbenet
Copy link
Member Author

lbenet commented Feb 22, 2018

I also thought so, but github thinks it can be merged directly. In any case, it is ready for review and also to be merged.

@dpsanders dpsanders force-pushed the lb/integrate_symbols branch from c6aa762 to 41dc075 Compare February 23, 2018 19:12
src/calculus.jl Outdated
@@ -131,6 +131,8 @@ function derivative(a::HomogeneousPolynomial, r::Int)

return HomogeneousPolynomial{T}(coeffs, a.order-1)
end
derivative(a::HomogeneousPolynomial, s::Symbol) =
derivative(a, findfirst(get_variable_symbols(), s))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should introduce a separate function to do the variable lookup in case we decide to change the mechanism later.

Copy link
Member Author

Choose a reason for hiding this comment

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

So the proposal is to have something like lookupsymb(s) which is equivalent to findfirst(get_variable_symbols(), s). Is this what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes exactly. Maybe lookupvar

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok.

@dpsanders
Copy link
Contributor

Note that I just rebased against master.

@lbenet lbenet mentioned this pull request Feb 23, 2018
@lbenet
Copy link
Member Author

lbenet commented Feb 23, 2018

Just pushed another commit following your suggestions

@lbenet
Copy link
Member Author

lbenet commented Feb 23, 2018

Incidentally, I did not export lookupvar; do you think it is needed?

@dpsanders
Copy link
Contributor

I think it's fine (or better) if it's not exported.

@dpsanders
Copy link
Contributor

LGTM, thanks!

@dpsanders dpsanders merged commit 5007129 into master Feb 23, 2018
@dpsanders
Copy link
Contributor

Thanks!

@lbenet
Copy link
Member Author

lbenet commented Feb 23, 2018

Thanks a lot!

@lbenet lbenet mentioned this pull request Feb 23, 2018
2 tasks
@lbenet lbenet deleted the lb/integrate_symbols branch March 10, 2018 04:10
@lbenet lbenet restored the lb/integrate_symbols branch March 10, 2018 04:12
@lbenet lbenet deleted the lb/integrate_symbols branch March 10, 2018 04:18
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.

Partial evaluateion Allow differentiation and integration with respect to symbolic variable names
3 participants