Skip to content
This repository was archived by the owner on Oct 9, 2019. It is now read-only.

Return lower order results #94

Merged
merged 6 commits into from
Dec 14, 2016
Merged

Return lower order results #94

merged 6 commits into from
Dec 14, 2016

Conversation

fredrikekre
Copy link
Collaborator

@fredrikekre fredrikekre commented Dec 13, 2016

Fix #92 perhaps

This returns the value of the function by default. How can we do this for hessian? If we do it the way we do it now that would mean we load the tensor twice, and extract the result twice...

TODO:

  • Update tests
  • Update docs
  • Run benchmarks

@KristofferC
Copy link
Owner

KristofferC commented Dec 13, 2016

This is pretty much exactly what I had planned to do. Nice :). I don't think loading and unloading the second order tensor twice will be a problem compared to having to do it for the Hessian.

For Hessian I thought something like this could work?

function hessian{F}(f::F, v::Union{SecondOrderTensor, Vec})
      s = zero(eltype(v))
      gradf = y -> begin
         s, fv1 = gradient(f, y)
         return s, fv1
      end
      fv, ∇fv = gradient(gradf, v)
      return s, fv, ∇fv 
end

This might give bad performance because julia sometimes have problems when you modify a variable in a closure.

@fredrikekre
Copy link
Collaborator Author

fredrikekre commented Dec 13, 2016

Didnt see your solution until I pushed mine...

The problem with yours is that gradf returns 2 things, and that doesn't work with gradient.

@fredrikekre
Copy link
Collaborator Author

But I guess your gradf can be modified to just return fv1, but will s still be 0 then?

v_dual = _load(v)
res = f(v_dual)
fv, ∇fv = _extract(res, v)
return ∇fv
Copy link
Owner

Choose a reason for hiding this comment

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

return fv, ∇fv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no? this is not gradient, its _gradient, I use this only in hessian since we need a version of gradient that returns only the gradient and not the value.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, sorry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But might not be the best solution though

@fredrikekre
Copy link
Collaborator Author

Tests should pass now, but maybe we should think a bit more on the hessian. Will do some other stuff now.

@fredrikekre
Copy link
Collaborator Author

Maybe we should reverse the order as well? You would expect that a function called gradient should first and foremost return the gradient, and then perhaps some other stuff.

OTH with the order we have now the value is always the first output, regardless if it is gradient or hessian, and the gradient is always output 2 for both gradient and hessian as well.

Thoughts?

@@ -89,7 +89,7 @@ end
@inline function _extract{D <: Dual}(v::Tensor{2, 2, D}, ::Any)
@inbounds begin
v1, v2, v3, v4 = value(v[1,1]), value(v[2,1]), value(v[1,2]), value(v[2,2])
f = Tensor{2, 2}((v1, v2, v3, v3))
Copy link
Owner

Choose a reason for hiding this comment

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

kappa

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thats already fixed 👍

@fredrikekre
Copy link
Collaborator Author

Oh, right... forgot to update the doctests :) Will do it later.

@fredrikekre
Copy link
Collaborator Author

Seems like it is about two times slower for both hessian and gradient, but it is a bit unfair to compare since the new functions calculate the values too I guess.

@fredrikekre
Copy link
Collaborator Author

Will make some better benchmarks later.

@KristofferC
Copy link
Owner

You can run only the AD benchmarks with run_benchmarks("ad_bench", @tagged "ad")

@fredrikekre fredrikekre force-pushed the fe/lower_order_results branch from d8fe6e9 to 1ca466d Compare December 13, 2016 12:48
@fredrikekre fredrikekre force-pushed the fe/lower_order_results branch from 1ca466d to 057c538 Compare December 13, 2016 12:50
@fredrikekre fredrikekre added the AD label Dec 13, 2016
@fredrikekre fredrikekre changed the title [RFC] Return lower order results Return lower order results Dec 13, 2016
@fredrikekre
Copy link
Collaborator Author

@inferred tests fails on nightly :/

@KristofferC
Copy link
Owner

Minimized to:

julia> using StaticArrays

julia> s = rand(SVector{3})
3-element StaticArrays.SVector{3,Float64}:
 0.710116 
 0.382896 
 0.0640471

julia> s * 0.0
3-element StaticArrays.SVector{3,Float64}:
 0.0
 0.0
 0.0

julia> @code_warntype s * 0.0

@fredrikekre
Copy link
Collaborator Author

same with just rand too:

@code_warntype rand(SVector{3})

@KristofferC
Copy link
Owner

Maybe JuliaLang/julia#19421

@KristofferC
Copy link
Owner

Only the rand looks good to me.

@KristofferC
Copy link
Owner

The docs (not docstrings) should perhaps say something about the :all.

@fredrikekre
Copy link
Collaborator Author

Sure, btw, is it too verbose with the examples in the docstrings? Maybe we can skip examples for the :all version?

@KristofferC
Copy link
Owner

The hessian is a bit verbose, yeah. Could maybe just assign them to variables and not print the result, f, ∇f, ∇²f = hessian(f, S, :all)?

@fredrikekre fredrikekre force-pushed the fe/lower_order_results branch from 8a01749 to cdef41a Compare December 13, 2016 17:14
```@docs
gradient
hessian
```

We here give a few examples of differentiating various functions and compare with the analytical solution.
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps make this into a header not that the section is becoming quite long. ## Examples

@KristofferC
Copy link
Owner

Anything left to do here?

@fredrikekre
Copy link
Collaborator Author

No, not necessary to run benchmarks, since we now left the original functions untouched.

@fredrikekre fredrikekre merged commit 491d2aa into master Dec 14, 2016
@fredrikekre fredrikekre deleted the fe/lower_order_results branch December 14, 2016 08:32
@KristofferC
Copy link
Owner

Wasnt docstrings supposed to be inserted here https://kristofferc.github.io/ContMechTensors.jl/latest/man/automatic_differentiation.html ?

@fredrikekre
Copy link
Collaborator Author

@KristofferC
Copy link
Owner

Docs run on nightly which are failing? Change docs to run on v0.5?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants