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 bug with typed_fad_gradient!() #21

Closed
wants to merge 4 commits into from

Conversation

utkinis
Copy link

@utkinis utkinis commented Mar 28, 2015

Current implementation of typed_fad_gradient!() contains a bug related to access to non-existent field of a structure and is inconsistent with typed_fad_gradient(). When trying to call forwarddiff_jacobian!() of a function that returns Vector, an error occurs. In this example:

using ForwardDiff

F(x) = [x[1]^2, x[1]*x[2], x[2]^2]

J! = forwarddiff_jacobian!(F, Float64, fadtype=:typed)

A = zeros(3, 2)
J!([1., 1.], A)

after execution got

`g!` has no method matching g!(::Array{Float64,1}, ::Array{Float64,2})

If try to make A=zeros(6) got

type Array has no field g

After that fix, one can use Matrix of the same shape as in the output of forwarddiff_jacobian().

@papamarkou
Copy link
Contributor

Thanks for the fix @krjackvinator, I will look at this in a few days as I am currently travelling.

@papamarkou
Copy link
Contributor

@krjackvinator apologies for taking me a while to look at this, do you want to fix your PR so that it doesn't fail, and then I can check it and merge it?

@utkinis
Copy link
Author

utkinis commented Jun 17, 2015

@Scidom i apologize for not replying for so long time(i had been working on my graduate thesis). I had to change tests a bit for forwarddiff_jacobian!() because this function should take type Array{Float64, 2} instead of Array{Float64, 1}(it doesn't know if argument function returns one value or array of Float64). Unfortunately, now this PR contains merge conflicts. I could merge master with my branch, but then commits from master will get into PR, so i'd better wait for your decision.

@jrevels
Copy link
Member

jrevels commented Aug 14, 2015

This now fixed on master as of the merging of #27.

@jrevels jrevels closed this Aug 14, 2015
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.

3 participants