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

Rework Problem structure to have a Objective for reusing interim computations and a Cache #174

Merged
merged 154 commits into from
Jan 10, 2023

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Dec 2, 2022

This is a little bit of a (slowly progressing) rework of the Problem structure to enable that a cost and a grad can be the same function and we might be able to cache interims results. This resolves #172.

Features introduced in Manopt 0.4

  • Rename Problem -> AbstractManoptProblem
  • Introduce an AbstractManifoldObjective to store all information about the objective
  • Have a DefaultManoptProblem when it (just) constains the manifold and an AbstractManifoldObjective
  • Introduce new objectives for the old problems, most of the become <:AbstractManifoldObjectives within a new DefaultManoptProblem (which has the manifold and the objective).
    • AlternatingGradientProblem is now a ManifoldAlternatingGradientObjective
    • CostProblem now is a ManifoldCostObjective
    • GradientProlem now is a ManifoldGradientObjective
    • StochasticGradientProblem is now a ManifoldStochasticGradientObjective
    • SubgradientProblem is now a ManifoldSubgradientObjective
    • PrimalDualProblem is now a TwoManifoldProblem with two new cost structures
    • Frank Wolfe Problem
    • ConstrainedProblem is now a ConstrainedManifoldObjective
    • ProximalProblem is now a ManifoldProximalMapsObjective
    • HessianProblem is now a ManifoldHessianObjective
    • NonlinearLeastSquaresObjective is now a NonlinearLeastSquaresObjective
  • Rename Options to AbstractManoptSolverState
  • Rename solve to solve!
  • be more consistent with the variables following the https://juliamanifolds.github.io/Manifolds.jl/latest/misc/notation.html
  • rework docs
    • update all docs
    • rework the solvers start page to give an easier access to which solver to use when.
    • check why a few links are still broken
    • Check Tutorials on 0.4.
  • remove old @deprecated constructors (those without manifold first – so without manifold defaults)
  • Direction update rules (post processors for gradient descent directions like AverageGradient) now have a unified constructor, the manifold is the only mandatory argument, all others are kwargs.
  • unify usage of FieldReference in the accessor functions.
  • Introduce a Caching idea – SimpleCacheObjective that stores one recent point and/or gradient in a storage, resolves Make DebugWarnIfCostIncreases not call objective when possible #171
  • remove random_point and random_tangent in favour or the rand functions from Manifolds.jl (if that is loaded), resolves random_point and random_tangent #178
  • Introduce a default_stepsize(M, mp) that returns a default stepwise functor based on the Manifold and problem combination, resolves Introduce default_stepsize(M, O) #180
  • Introduce a ManifoldCostGradObjective with one function computing cost and gradient simultaneously A combined CostGrad structure #139
  • Think about a generic CachedManifoldObjective with a generic cache with more than one value stored. postponed to later.
  • Refactor check_gradient to return the figure but not show it necessarily. That way the plot can be suppressed.
  • wrap warnings in tests so they are captured when checked
  • depend on ManifoldDiff and remove the corresponding functions from here (resolves Differentials and adjoint differentials in ManifoldDiff.jl #177)
  • refactor Nelder Mead to be more performant (resolves Nelder-Mead seems to be much worse than in Optim.jl #179 and Add simplex size termination condition to Nelder-Mead #183)
  • write a small test for the cache and give text coverage a final check

qrebjock and others added 26 commits March 24, 2021 17:48
# Conflicts:
#	src/plans/hessian_plan.jl
#	src/solvers/truncated_conjugate_gradient_descent.jl
#	src/solvers/trust_regions.jl
* Rename Problem to AbstractManoptProblem
* Introduce an AbstractManifoldObjective
* Rename solve to solve!
@kellertuer kellertuer marked this pull request as draft December 2, 2022 08:18
@kellertuer
Copy link
Member Author

Status I: After introducing the new structure, the cost problems (Nelder Mead, particle swarm) seem to work already with only reasonable rework of code.

@kellertuer
Copy link
Member Author

kellertuer commented Jan 7, 2023

I like Pluto in general, just within docs, especially with tutorials that should be up to date to the most recent version that is a hustle and the results don't look that nice. Even more – one of the notebooks sometimes stalls and I can‘t figure out why, because it only happens on CI and there is never any feedback / message.

For ManoptExamples I might use mainly Pluto Notebooks to illustrate how the examples work, since there its fine if that refers to an old version.

I fixed the docs and even see roughly how I could fix the remaining 13 lines of coverage – but I also would like to finish this somewhen.

Sure a test with the PCA would be great, as soon as the ManifoldDiff PR is merged we can check here what we can remove when we introduce the dependency.

@kellertuer
Copy link
Member Author

kellertuer commented Jan 8, 2023

I started a-roping most of the code to ManifoldDiff – however my diff_forward_logs, which basically is the differential of foraward logs on an array of points now fails with one very strange number in its entries and I get one allocation error from ManifoldsDiff when calling adjoint_Jacobi_field from ManifoldDiff.

For neither of them I have a clue where that comes from nor how to solve that (spent the last half hour on them).

edit: Ah, I see the new adjoint Jacobi fields do not yet work on manifold like Circle or PositiveNumbers or other non-mutating variants. Could we add that?
Then just the strange error in Forwardlogs persists, which should be zero but isn't.

kellertuer and others added 3 commits January 8, 2023 09:18
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@kellertuer
Copy link
Member Author

Found my bug – now the only thing left is Jacobi fields for circle and such...

@kellertuer kellertuer marked this pull request as ready for review January 8, 2023 11:43
@kellertuer
Copy link
Member Author

For the remaining tests here to pass, we would need JuliaManifolds/ManifoldDiff.jl#11, but the rest is done.

@mateuszbaran
Copy link
Member

Sure, I will fix nonmutating manifolds in a moment.

@mateuszbaran
Copy link
Member

Hm, it doesn't look like Jacobi fields actually work for PositiveNumbers. I will just fix Circle and Euclidean for now.

@mateuszbaran
Copy link
Member

That is, there is no diagonalizing basis for PositiveNumbers currently.

@kellertuer
Copy link
Member Author

Ok, that basis is a very boring one, but sure, my main application is actually the circle, though I covered PositiveNumbersand Euclidean{Tuple{}} in my non mutating cases as well.

@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Jan 9, 2023
Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

I think caching requires some additional work but I don't really have enough time to work on it now. It will have to wait. In the meantime maybe it would be a good idea to add a note that it's an experimental feature for now?

@kellertuer
Copy link
Member Author

I think the simple cache should be fine, but sure, that is just a first step and we could also call that experimental for now.

@kellertuer
Copy link
Member Author

I also opened #192 concerning the next steps in Caching.

@kellertuer kellertuer merged commit 9a8d127 into master Jan 10, 2023
@kellertuer kellertuer deleted the kellertuer/costgrad-and-cache branch December 16, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment