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

Transfer DualNumbers implementation from ForwardDiff #49

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ranjanan
Copy link

Well, I looked at #45, so I thought this PR might help. The code is basically the same, but I exported Dual, value and partials. I also added pretty printing, so Dual Numbers would look like:

julia> d = Dual(4, 1, 3)
4 + 1ϵ₁ + 3ϵ₂

@KristofferC
Copy link
Collaborator

I think we should definitely try to keep the git history for these files.

@jrevels
Copy link
Member

jrevels commented Feb 13, 2017

Thanks @ranjanan. Before you go down this route, be warned that it will probably involve a good bit of work and digging into the implementation details of both packages. I had planned on doing this work myself after v0.6 released to avoid having to continuously update with breakage/depwarn fixes.

We need the change-over to not merely swap out the old implementation for ForwardDiff's, but to ensure that the feature sets of the two implementations are appropriately merged. We'll wish to drop some of the old behaviors, while other behaviors we'll wish to preserve, probably requiring new definitions. For example, there are some primitives defined on DualNumbers.Dual that are not yet defined on ForwardDiff.Dual. There might be more subtle behavioral changes as well.

Things that have to be done (besides just porting over the code):

  • Implement a deprecation layer
  • Implement whatever new functionality we need to appropriately merge the behavior of the two implementations
  • Write new tests for any new definitions
  • Write documentation describing the new interface

I'd also like my name added to the LICENSE (and I believe @mlubin is also within his rights to request this, but I'll let him speak for himself). I believe doing this requires Theo's permission?

Some comments on this PR as it is:

  • I'm not sure we want to export anything (especially such a generic function name like value), might be better to let users import these themselves. But I'm usually more conservative in this area ever since dealing with the whole Base.gradient confusion 😛
  • As somebody working with Duals quite a lot, I strongly prefer ForwardDiff's current pretty printing. See my comment here.

@ranjanan
Copy link
Author

@jrevels Thank you for your comments. I shall work to complete those objectives. I'd be most grateful if you would comment as and when you see something worth discussing, such as an API that's being deprecated, etc.

Go back to original ForwardDiff's Dual pretty printing
[ci skip]
* conjdual
* absdual
* abs2dual

[ci skip]
[ci skip]
@ranjanan
Copy link
Author

Seems to be failing because of travis-ci/travis-ci#4942, adding the following fix (travis-ci/travis-ci#4942 (comment)) to .travis.yml

@ranjanan
Copy link
Author

This fails because the function erf has been moved to SpecialFunctions.jl on v0.6

@hyrodium
Copy link

What is the status of this PR? Is this just waiting for resolving the conflicts?

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.

4 participants