Separate State and StoppingCriterionState initialization

[lkdvos]

This is a slight refactor of the initialization procedure, where I did the following

• Separate out the initialization of the state and stopping state, such you don't need to remember to call initialize_state! on the stopping state.
• Update the docs to include a custom stopping implementation with and without stateful stopping criterion
• Update the docs and tests to no longer reset the iterate in the initialize_state! calls, and only generate a random starting point in initialize_state as this led to confusion in solve! without initialize_state! #8. The idea is that initialize_state! really means making sure everything is set up to correctly start running the algorithm, which does not necessarily include a reset of the iterate but rather means setup like caches, counters, auxiliary memory etc.

[lkdvos]

@mtfishman, does this align slightly better with what you had in mind?

[codecov]

Codecov Report

❌ Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@c0ad8e3). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/stopping_criterion.jl	70.00%	3 Missing ⚠️
src/interface/stopping.jl	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main       #9   +/-   ##
=======================================
  Coverage        ?   57.70%           
=======================================
  Files           ?        6           
  Lines           ?      227           
  Branches        ?        0           
=======================================
  Hits            ?      131           
  Misses          ?       96           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lkdvos]

https://juliamanifolds.github.io/AlgorithmsInterface.jl/previews/PR9

[kellertuer]

Oh, this is quiiiiiite some rework. I do not get understand what changed in practice though. It seems like now it's random whether initialise_state has an actual (algorithm) state or a stopping criterion state as third parameter? Is that the new part?

Or phrased differently: one now has to pass a sc-state to the init, but where is that from / initialised before?

[lkdvos]

Apologies, I could have explained this a whole lot better.

The main goal was to separate out state initialization from stopping state initialization, since I think it feels slightly awkward that you have to remember that when you are implementing an algorithm, you have to remember to also call initialize_state(!) for the stopping criterion.
Since this is something that always has to happen anyways, I tried separating that out into initialize_stopping_state! and initialize_state! in the solve! function, such that this is handled automatically.

Then I wanted to do the same thing for initialize_state and initialize_stopping_state, but since we have to attach the stopping_criterion_state into the generate state, I still have to pass that as an argument to the initialize_state, which then takes the stopping_criterion_state and generates a state from that.

Walking through the flow of solve(::Problem, ::Algorithm), we therefore have:

1. initialize_stopping_state(problem, algorithm, stopping_criterion = algorithm.stopping_criterion) -> stopping_criterion_state
2. initialize_state(problem, algorithm, stopping_criterion_state) -> state
3. enter solve!
4. initialize_stopping_state!(problem, algorithm, state, stopping_criterion = algorithm.stopping_criterion, stopping_criterion_state = state.stopping_criterion_state)
5. initialize_state!(problem, algorithm, state)
6. remainder of the algorithm

I can see the confusion with the third parameter being a state or stopping_criterion_state, but this is mostly because the signatures of the ! methods don't line up nicely with their non-mutating counterparts, and I still wanted to pass on all the parameters to the different functions in case any implementation requires access to that.

Finally, I wanted to not disallow workflows where the initialization of the stopping criterion state and of the regular state have to be intertwined and cannot be cleanly separated out.
To achieve this, I am only calling initialize_state(problem, algorithm) in the solve call, and have a default implementation that dispatches to initialize_state(problem, algorithm, initialize_stopping_state(problem, algorithm)).

[mtfishman]

It's a small change, but in practice what I've been doing is writing the definition of initialize_state more like this:

function AlgorithmsInterface.initialize_state(
        problem::SqrtProblem, algorithm::HeronAlgorithm,
        stopping_criterion_state::StoppingCriterionState;
        x0 = rand()
    )

Maybe I was being dense, but it took me some time to realize that was a "valid" way to define it and still have control over the parts of the state initialization that I wanted to control, since that allows running solve as:

solve(SqrtProblem(16.0), HeronAlgorithm(StopAfterIteration(10)); x0 = 1.0)

I think seeing the example written that way would have helped me understand how I should define initialize_state.

However, I realized a slightly awkward thing about defining initialize_state in that way and then calling solve as solve(SqrtProblem(16.0), HeronAlgorithm(StopAfterIteration(10)); x0 = 1.0) is that then x0 gets passed to both initialize_state and initialize_state!. Maybe that's not a problem in practice but I found it to be a bit strange (i.e. should initialize_state! use x0 or not?), and also made me confused about the roles of intialize_state vs. initialize_state!. I think the suggestion in this PR of just having initialize_state! reset iteration helps clarify that issue a bit.

[lkdvos]

The comment about the kwargs is a really good one, I was looking at this a bit before and it's somewhat difficult to come up with a way to split arbitrary keywords generically, but it could even make sense to just not pass the keyword arguments to initialize_state! anymore if they have already been passed to initialize_state?

[mtfishman]

I was thinking about that, indeed maybe it makes sense to not pass the keyword arguments to initialize_state!. That could make the distinction between initialize_state and initialize_state! clearer, since then initialize_state handles external inputs (i.e. initial guesses for the iterate) while initialize_state! just handles resetting the "internal" state, such as the iteration number and stopping criterion state. Calling solve! means that you made the state already, so it seems like anything handled through keyword arguments to initialize_state! could instead be handled by just modifying the state directly (i.e. before calling solve!/initialize_state!).

[kellertuer]

Hm, I disagree here, for me functions of same name should do the same and accept the same keywords. Or to be more precise

initialize_state should create a state from new memory but also
initialize_state! applied to the same memory state should set it to the same situation as well.

[kellertuer]

I think for me all this is getting a bit too much.

My original goal was to maybe integrate Manopt a bit better into the Julia world so people could use it. That failed with JuMP and it also failed with Optimization.
This package here was the idea to integrate it differently, but by now I noticed, the form we ended up with would mean to basically rewrite Manopt from scratch. That is some 10k lines of code.
I did rewrite LieGroups.jl last winter and I am currently rewriting the Manifolds.jl tests. Afterwards everything is a bit nicer, but it takes a huuuuge effort of time.
I will not do that further I think. So Manopt will probably not switch to this. You seem to have super good ideas and use cases already, so should we then maybe move it to some TensorKit namespace and you continue this overall?

I do not mean this in any negative way, I think it was nice to ponder about these ideas a bit, I just notice that I do not have the capacity to continue here - neither in motivation nor time. Nor does it look like I will be able to afford any future JuliaCons anyways (celebrating 10 years without grants by now).

Let me know what you think. Without me you can also easily follow the ideas above of doing something completely different in the mutating versus non mutating variants of functions and such.

[lkdvos]

I completely understand what you mean, that really is a significant amount of dev time that at least for the case of Manopt has a smaller return on investment as you already have most of the features working.
If you think this is better, I would happily "adopt" this repository, we could for example move it to the QuantumKitHub organization and we'll see where to go from there.
In any case I am grateful for the discussions and your input, I know that that, in combination with the inspiration from the Manopt ecosystem has at least greatly helped me get a grasp on what I would want from this and how to achieve it.

As you may have noticed, for me as well this is something that I am working on slightly sporadically, but at the very least I am not too happy with the current setup in a large part of our ecosystems so this is less of a refactor and more of a feature improvement, so there is slightly more motivation for that.

[kellertuer]

Just to be clear here:

• I still think this is a neat idea and nice to have
• I still think the design we ended up with is quite nice and flexible

Just compared to my initial idea (“extract” the main types from Manopt and make it a package), the current design would really mean a major major rework of Manopt.jl.
I do not mind reworks, but I feel I mainly did that recently.

I also value our discussions, I think we got to something really nice. Just that I lack a bit of motivation. If you are fine with that, we can also keep it here in the JuliaManifold ecosystem, just that for now I do not see that Manopt will be rewritten to that; maybe unless there pops up some large argument that I find the motivation suddenly; but its a refactor of (I think) about 70% of the code lines.
So I do not want to give a wrong impression, when the package lives here. But if it is fine with you we can also just keep it here :)

I am also happy to help in discussions – just for now I am not sure what else I could (find the motivation to) contribute.

[lkdvos]

Ah, I misunderstood then, I thought you had wanted to migrate the package. I'm totally fine with keeping it here, I guess it just wasn't totally clear whether you wanted to not have to spend any time on this, or simply be kept up to date, or feel like chiming in on the design as well. Obviously I prefer to have you on board, I just don't want to force you to spend time you don't have 😉.

[kellertuer]

Ok. You are right I was thinking for a moment about moving the repository, since as of now it will probably have no relation to the org it is in. But well, that is also not too bad.
I think I currently miss the motivation to contribute code here.

For now, I can help in discussions though not here, I do not follow the changes and what they mean for how the interface is used. What I do not like is that there are now two functions, initialize_state and initialize_state!that- as far as I follow along here, do something completely different, though they have the same name? I think that should not be the case.

Having the same name for me means: They should do exactly the same things, just that the first creates a state and does the things, while the second does the same things on an already existing state. That should include all arguments and keywords and everything the functions do.

[mtfishman]

@kellertuer I agree in general that x = f(args...; kwargs...) and f!(x, args...; kwargs...) should result in the same x. I think what threw me off about the current design is that when you call solve, both initialize_state and initialize_state! get called, which doesn't seem right to me, especially if they do some non-trivial work. My initial thought was that maybe initialize_state! could just not be called from solve! (after all, users can call it manually before calling solve! if they want to), but then @lkdvos pointed out to me that it is important to reset the stopping criterion state, so that led to the idea of having a separate initialize_stopping_state!. Maybe we could go with a design where only initialize_stopping_state! gets called from solve!?