Start with the main types

[kellertuer]

Here are the first few steps from our discussion

📋

• introduce the main types (Algorithm,Problem, State)
• set up docs
• setup tests
• start first two functions
• maybe get a first StoppingCriterion started
• add a first test as proof of concept.

💬

A sorted list of the discussion points below – with (L)etters to refer to, to make the whole discussion a bit more structured – and a checkbox to mark to indicate when the discussion is both finished and realised in code

• (A) order of arguments: (problem, algorithm, state) or state, problem, algorithm)?
• (B) solve! should return the state it modified
• (C) Split also stopping criterion into static (parameters) and variable (caches) structs
• (D) stopping criteria are currently functors, this should be changed since then
  • is_finished can just return the current answer while
  • is_finished! would also update internal state, e.g. also store the last iterate again
    that way also internal things could access this again without modifying the stopping criterions state (it would call is_finished, update state, return result)
• (E) we should discuss the signature of is_finished
  computing change
• (F) naming accessors: get_X / is_X or just X? (currently get_X is used to allow for set_X! as well
• (G) use full variable names, i.e. algorithm instead of just a
• (H) change get_reason to return nothing instead of an empty string, when not yet finished
• (I) use Base.summary to show the summary of a state

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[kellertuer]

@lkdvos besides testing this on a small dummy algorithm in the test suite, this should be ready so far.

A next step would be to boost stopping criteria:

• StopWhenAny and |
• StopWhenAll and &
  and other fancy ideas I have – but I would like to take that step by step and only define the main types here, first.

Let me know what you think about the current start both in code and docs.

[lkdvos]

I started adding a dummy tests which currently don't work.
Here are some things that I ran into:

• I think I would prefer initialize_state!(state, problem, algorithm) instead of initialize_state!(problem, algorithm, state), following the usual mutate first argument convention, and similar for step!, solve! and other in-place methods, if possible.
• Currently, solve and solve! return nothing . We could simply return state, butmaybe we can add an interface function to convert things into a solution, which is by default called at the end. Something like:

function solve!(p::Problem, a::Algorithm, s::State)
    state = initialize_state!(s, p, a)
    while !isfinished(p, a, s)
        state = step!(s, p, a)
    end
    return get_solution(s, p, a)
end

• As you indicated, the stopping criterium is now duplicated, because I did not anticipate that the stopping criteria hold state. Brainstorming a bit here, because I am not yet sure if I like this idea myself:
  I feel like what I thought of as the stopping criterium is simply:

struct StopAfterIteration
    max_iterations::Int
end

ie, the information that tells you what kind of criterium (through its type), and the settings for that. This is the information that is static, and would therefore belong in the algorithm.
The at_iteration information, as well as the dynamic initialization at iteration 0 was what I failed to think about, and a bit what surprised me. This seems somehow not entirely related to the stopping criterium, and is mostly information about the stopping state, so maybe we can split these?
As I said, I'm not sure how much I like this idea, it might also become quite a bit of boiler plate to deal with that.
The main worry I have with these stopping criteria however is that they don't multithread very well: if I wanted to do a parameter sweep of something, I would often write:

alg = my_algorithm_settings(; kwargs...)
for param in all_parameters
    problem = Problem(param)
    solve(problem, alg)
end

If I now change for for OhMyThreads.tforeach, I need to remember that I need to explicitly copy my stopping criterium, otherwise things will fail and cause hard to track bugs.

• How do you feel about changing the get_someprop(f) functions to simply someprop(f)?
• I'm not completely convinced by enforcing the split of step! and increment!. Do we want to enforce strict linear iterations? For example, what do we do for things that have both inner and outer iterations, such as many of the Krylov algorithms?

[kellertuer]

Interesting discussion points! Though especially with the stopping criterion (SC) I fear we are much further away from a first prototype than I thought.

• For the first point. I think that is a bit of a trade-off. If we always put the mutated argument first, we might end up with arbitrary permutations (p, a, s) (default or if p is modified), (a, p, s) (for example in a update_algorithm – or (a,s,p)?) and the one you propose. Can we make that somehow clear to the user to not end up with all 8 possible permutations?

• For the second – sure, both should return state, I just missed those lines.

• For the stopping_criterion – the current form is the one I do in Manopt.jl, since that way a StoppingCriterion is a functor that also remembers whether it was active – mainly to report that at the end.
  In general we could split these into a StopAfterIteration(10) that would just store maxIter and a StoppingCriterionState{SC<:StoppingCriterion} type or so, but then implementing that as a functor with dispatch would probably be hell. Especially because some of those states would need a bit of knowledge on the side (like a last iterate or so if they check for change)
  I am also not so sure how that would interact / work then with “comnbining” these easily. The old form with one structure allowed to do the StopWhenAny to just store a tuple of SCs and the functor would iterate through these; if one is true, it would return true. If that is now 2 things, we need 2 any-things...? Not sure yet. This might become very very very boilerplate heavy to split that.
  But I do agree that they have a static information (stopping after k iterations) – and a variable one (should we stop now? Report/indicate these statuses).
  And actually nearly the same for e.g. line searches like Armijo. So I fear how to model/handle this is a really crucial thing to decide. I also have zero experience on threads....

• I do prefer get_X and is_X I think, since there is also set_X – like in get_iterate(s) but also set_iterate!(s, x), how would your naming scheme handle that?

• for step! and increment! I am not sure. My old code had just k instead of state and the while loop incremented that hard coded. My idea was, that increment! makes that a bit more flexible. My old states also did not store that, so only the solve! method “knows” the k (but also passed it to solve)
  What I wanted to avoid is, that every solver has to do a manual line s.iteration +=1 in their step!-function. That would be super-repetitive (when looking at all algorithms). This would need to be documented as well and seems very redundant to have that line in every step! function.

[kellertuer]

I thought a bit about the SC. I can give the following idea a try tomorrow, though for combined ons I am not yet so convinced ;)

We take

struct StopAfterIteration <: StoppingCriterion
    max_iter::int
end
struct StoppingCriterionState{SC <: StoppingCriterion}
    stop::SC
    at_iteration::Int
    # maybe a generic cache structure here?
end

This could still be a functor, maybe parametrized – that should work. The tricky part is what to do with a

StopWhenAny{SCs <: NTuple{<:SC, N}}
    stopping_criteria::SCs
end
```

Because for this creating the corresponding SCState would mean we need a tuple of SCStates? Each single one should still be able to tell when they “indicated” and report their reasons, because maybe someone wants to know which criteria were the reason to stop, then it is important to see _two_ indicated. Similarly for `StopWhenAll`, something that has a tuple of ints instead of an int for the `at_iterations`?

Will think about whether maybe one `StoppingCriteriaState` thingy can handle this.

[lkdvos]

If we always put the mutated argument first

Although I guess the mutated argument should always be the state, since the other information is static? Basically, maybe we could have ([state], problem, algorithm) everywhere?

The increment is definitely also tricky: not enough utility and we end up with a lot of boilerplate, too much and we lose flexibility. Giving it a second thought though, we can still have this as opt-out: defining increment!(::MyType) = nothing and manually handling that should also work?

---

For the stopping criterium state, we could also do what you suggested, with less enforcement on the type of the states a stopping criterium has:

struct StoppingCriteriumState{SC<:StoppingCriterium,SS}
    stopping_criteria::SC
    stopping_state::SS
end

StoppingCriteriumState(criterium::StopWhenAny) = StoppingCriteriumState(criterium, StoppingCriteriumState.(criterium.stopping_criteria))

in other words, really having a tuple of states.

[kellertuer]

If we always put the mutated argument first

Although I guess the mutated argument should always be the state, since the other information is static? Basically, maybe we could have ([state], problem, algorithm) everywhere?

Would just be a bit unusual for me, since I ordered that the other way around for 9 years now, but well.... you are right there is not so much variation when all 3 appear. We could then also go for

• non mutating cases (p, a, s)
• mutating cases (s, p, a)

The increment is definitely also tricky: not enough utility and we end up with a lot of boilerplate, too much and we lose flexibility. Giving it a second thought though, we can still have this as opt-out: defining increment!(::MyType) = nothing and manually handling that should also work?

Sure we can have that as an opt-out.

For the stopping criterium state, we could also do what you suggested, with less enforcement on the type of the states a stopping criterium has:

struct StoppingCriteriumState{SC<:StoppingCriterium,SS}
    stopping_criteria::SC
    stopping_state::SS
end

StoppingCriteriumState(criterium::StopWhenAny) = StoppingCriteriumState(criterium, StoppingCriteriumState.(criterium.stopping_criteria))

in other words, really having a tuple of states.

I will try both my and your idea the next few days, for me a crucial thing for the user to be able to state is something like

stopping_criterion = StopAfter(Minute(1)) | StopAfterIteration(100)`

but I do agree that that is static data for the Algorithm and hence not for the state. On the other hand both your and my approach “double” the static data into the state now.

So maybe also

(scs::StoppingCriterionState)(stopping_criterion, problem, algorithm, state)

os a good functor to have, so one can dispatch on the first argument mainly but has access to all other information. That looks reasonable to me – might still require different SCStates, but I can check.

[kellertuer]

After a longer walk this morning and about an hour of re-hacking a lot of things (instead of grading what I should have started with by now ;)) I followed the same route as with Algorithm and State also for stopping criteria and even noticed a few nice advantages

• StoppingCriterions subtypes can now even be structs and not mutable
• StoppingCriterionState is still a functor but for dispatch-reasons has the stopping criterion as fourth argument as well
• the DefaultStoppingCriterionState can even be reused for all stopping criteria that do not need anything in storage
• the one I use for the time measurement could now also be used to introduce a StopWhenSingleIterationExceeds stopping criterion (what ever that might be useful for
• I made the (problem, algorithm, state) versus (state, problem, algorithm) (for mutating) consistent and documented it

The main Idea I had was to reuse the initialize_state function also for the stopping criterions state – because for dispatch reasons with signature initialise_state(problem, algorithm, stopping_criterion) is necessary anyways but also nice, because the last type even determines which state to construct.

For the functor signature I also went for that order

(scs::StoppingCriterionState)(problem, algorithm, state, stopping_criterion)

instead of the one above – I am not yet sure what is the best order here; I feel the stopping criterion should probably either be first (most important there), but that would be a bit inconsistent with other signatures, or maybe be third (so before state) since it is the “smallest” part of the static variables.

But besides this question of argument order, I think I like the new more strict split between algorithm/static and state/dynamic date also in stopping criteria (though this will be heavily breaking for Manopt internals at least).
You are right it allows to really store the static configuration for an algorithm in one place.

Let me know what you think and what you feel about the argument order.
Once that is fixed I can update/fix the docs and finish this PR with testing that also StopWhenAny and StopWhenAll fit nicely in here. Both will share a StoppingCritertiaGroupState (or so), that is mainly a tuple of states (and an at_iteration).
So even there – sharing the state is possible and reduces a bit of code – nice!

[kellertuer]

For me the second is nice to read but I always feel like I am cheating when writing a new get_property function instead of an accessor. But maybe I am just not yet so used to overwriting that.

I think for now the things we want to provide access to are

• stopping_crtierion in Algorithm
• iterate (or x), iteration (or k), and stopping_criterion_state.

[lkdvos]

I tend to be okay with new property functions whenever this really is a property, ie a trivial amount of work is required to compute it, otherwise I feel functions are definitely more appropriate. From the language point of view it ultimately really doesn't matter, one is just a bit of syntactic sugar around the other so 🤷

[kellertuer]

Yeah, and I do not have so strong of a preference, maybe state.iteration and state.iterate is really nicer to read, so then let's go with your preference here and document carefully that others could also just overload get/set_property/I

[lkdvos]

To me this feels more or less in a state where I'd be okay with merging and starting to try it out.
I think we can definitely still make small or larger changes afterwards, but it's a bit hard to think about anything more without simply trying it out.
Perhaps we can look into cleaning up the readme and releasing a v0.1.0?
I would have also liked some infrastructure to handle logging, I'm not sure if we should do this before or after.

In any case, I'd do this in a separate PR and merge this one first though, that might be a bit easier to keep track of discussions etc.

[kellertuer]

Oh I think I would prefer to maybe at least try one state decorator like a debug (cf the Manopt debug) before a 0.1? But sure I can also do that in a 0.1.1.

I have to take a look at your property changes, but besides that I only have the remark I already posted.

[kellertuer]

I do agree, the state.iteration looks really nice in code, so that was a really good idea.

Before we release a 1.0 (but maybe not on this branch) I would like to

• get the docs rendered
• maybe add Aqua to tests (if not yet done)
• add badges to Reame
• check a bit for code cov, though we can do that as well when we test this interface along the way for the first few 0.x versions.

[kellertuer]

Nice! We are for now only at 45% coverage, but if you really want to try it already, sure we can do a first version.

I would prefer to wait a bit with my experiments and first at least get the debug and a first factory (to generate debug states) redone here. I could use such a PR also to work further on CodeCov.
Just not this week (end of the semester -> tired) nor next (vacation), unless I am bored while travelling ;)

[lkdvos]

Sure, so then let's merge this, but hold off on registration?

[kellertuer]

Yes. Could you do that? I am on mobile. Commenting is great but much more in GitHub ... a bit challenging.

[kellertuer]

Oh do we have Talbot and everything (JluiaRegistrator?) in place?

[lkdvos]

Looks like we do. I've left the docs action as is, since it still contains some of the quarto things, but I'm sure we can figure that out in coming commits/prs.

[kellertuer]

Quarto is for later tutorials and I was lazy to keep it in ;) But yes we can check that along the way.