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

Introduce the approximate Hessian as a default in trust regions. #237

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

kellertuer
Copy link
Member

...still might have to check for code cov.

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #237 (a1ac69f) into master (b6ba13f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #237   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          65       65           
  Lines        5553     5558    +5     
=======================================
+ Hits         5545     5550    +5     
  Misses          8        8           
Impacted Files Coverage Δ
src/plans/hessian_plan.jl 100.00% <ø> (ø)
src/plans/nonmutating_manifolds_plans.jl 100.00% <ø> (ø)
src/solvers/trust_regions.jl 96.33% <100.00%> (+0.21%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kellertuer kellertuer marked this pull request as ready for review April 12, 2023 11:15
@kellertuer
Copy link
Member Author

Tests here have to wait for JuliaManifolds/Manifolds.jl#592

@kellertuer kellertuer merged commit 96d6c85 into master Apr 13, 2023
@IWalds
Copy link
Contributor

IWalds commented Nov 1, 2023

Hi,
By curiosity, in trust_regions why did you impose that the type TH of Hess_f is a subtype of Function? I use this function by giving it a functor as an argument, which is not automatically a subtype of Function.
Thank you very much!

@kellertuer
Copy link
Member Author

Oh, we can relax that, sure. I can do that over on the #294 where I rework that anyways.

Can you wait a bit for that? The PR might take a week still.

@kellertuer
Copy link
Member Author

kellertuer commented Nov 1, 2023

Ah, I see why I did that. The Argument after the gradient can be p (a point) or the Hessian hess_f, so the ::Any case is (following the approach of Manifolds.jl) the point – not the Hessian. Since a function can not be a point on the manifold, hess_f is a subtype of function – otherwise the method signatures would be ambiguous.

So it will have to stay that way, sorry.

edit: But you can let your functor inherit from function, the approximate Hessian does that as well I think? Hm, I will check carefully whether I see a way to reduce the type.

@IWalds
Copy link
Contributor

IWalds commented Nov 1, 2023

Thank you very much for the explanation.
(Yes, I did let my functor inherit from Function; I was just curious why this change had been introduced.)

@kellertuer
Copy link
Member Author

I will still check whether I see a way to loosen that type a bit, but I am not sure yet.

@kellertuer
Copy link
Member Author

I carefully checked – it is not possible to loosen the restrictions on the hess_f since then we can not distinguish the hessian (optional) from a point (also optional) for trust_region.

One thing you can do, which is not too much more effort, is to generate the hessian objective before yourself and then call the solver, so

mho = ManifoldHessianObjective(f, grad_f, Hess_f) # precon is a positional argument, if you use `evaluation=` pass it here, too
# call then 
trust_regions(M, mho, p) #p is optional, if not provided p=rand(M) is used

I hope the comments cover the special cases you might use. Let me know whether that solves your problem.

@IWalds
Copy link
Contributor

IWalds commented Nov 1, 2023

Yes, generating the Hessian objective is also possible; I had not thought of it. It definitely solves my problem; thank you very much!

@kellertuer
Copy link
Member Author

That approach is originally so you can reuse an objective (here f+grad+hess) in different solvers easily, especially if it is with a cache and such – but here it also nicely can be used for a bit more advanced scenarios like yours. Glad that is helped :)

@kellertuer kellertuer deleted the kellertuer/trust-region-optional-hess branch December 16, 2023 07: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.

2 participants