Interface improvements: finalize_state! and deduplicate initialize_state(!) calls

[lkdvos]

This is a (long overdue) follow-up on some of the things that had been discussed but rejected in #9. The approach here is different, and I'm happy to hear some feedback.

Changes

• The first thing I changed here is that I added a finalize_state! function, which is called at the end of the solve(!) routines. The main point is to just have a hook that provides the option of returning something different than the state from those calls. For example, it might be reasonable that solve(problem, algorithm) -> solution, instead of returning a state that still has all of the internal details. Additionally this function allows for doing some clean-up, for example it might be used for releasing some resources or caches that were acquired, or maybe to write the result to disk, ...

• The second thing relates more to the previous closed PR, where I think one of the main comments was that solve ends up first calling initialize_state, and then calls solve!, which ends up calling initialize_state!. Since initialize_state and initialize_state! should probably do the same thing, this effectively means that solve does that work twice, which isn't necessary. Here, I bypassed that in the most simple way I could come up with: just duplicate the body of solve!. We might wish to discuss a bit more about this though, since this effectively means that if anyone decides to overload solve!, they must also overload solve for these to remain consistent. I think we should probably aim for it never really being necessary to overload solve(!) in the first place, so this might be fine, but I could be missing something.

[codecov]

Codecov Report

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

Files with missing lines	Patch %	Lines
src/interface/interface.jl	72.72%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #10   +/-   ##
=======================================
  Coverage        ?   53.71%           
=======================================
  Files           ?        5           
  Lines           ?      229           
  Branches        ?        0           
=======================================
  Hits            ?      123           
  Misses          ?      106           
  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.

[kellertuer]

The finalise is an interesting idea!

For now I modelled one of the aspects here the other way around:

Usually my Manopt.solve! would always end in returning get_iterate(state),
but Manopt has a ReturnSolverState <: State type (which wraps a state) and if that is present the corresponding solve! dispatch returns the state wrapped.

I think with finaliser this is a bit more generic and nicer so I like the idea.

[kellertuer]

Ah and thanks for coming back to this, I still think, having such a package would be very nice,
though I am still not sure where to find the time to rework manopt to fit the current design here.

And yes, the design here is a bit cleaner than the “grown over time” design of Manopt, but it is still a major rework that someone(tm) has to find time for. Maybe I will give it a try once we finish the 0.6 rework we currently do, which is by itself also adapting to 2-3 new solvers actually.

[lkdvos]

No worries, I think I will gradually keep trying stuff out when I have time as well, also because I just really value your feedback on this and it is a fun project, even though the priority isn't the highest :)

Usually my Manopt.solve! would always end in returning get_iterate(state),

I was thinking of that too, I would be totally fine with defaulting to finalize_state!(...) = state.iterate, as that does feel like it makes sense in most cases (and the function hook allows overloading so we're not forcing anything)

[kellertuer]

I think I would prefer to default it to an access function get_iterate(state) instead of the direct access with .iterate but besides that, having that as a default is fine with me; we could actually also directly do the ReturnState wrapper which overwrites finalise to return the state – and passes all other state functions to its internal state. For such a “wrapper” also get_iterate is necessary.

[lkdvos]

I think this is coming down to some discussions we were having earlier (which I am happy to revisit), but since it's been such a long time let me repeat some of my thoughts:

I tend to not like wrapper structs too much, mostly because these things often don't combine very well. If we are using the <:State type for dispatch, all of a sudden you also have to handle the case ReturnState{<:State}, and then if you also have a wrapper that stores to disk you get things like ReturnState{<:DiskState{<:State}} and DiskState{<:ReturnState{<:State}}, which very quickly becomes painful. (think of dealing with Adjoint, Transpose, SubArray, ReshapedArray, Diagonal, in all different combinations)
Therefore, if possible I would try to avoid that, which tends to favor functions instead of types. I do often feel like that is a more Julian approach, where wrapper types has a little bit of the object-oriented flavor and interface functions has the multiple-dispatch flavor. This is obviously subjective, so don't take this as a ground truth :)

For the distinction between get_iterate(state) and state.iterate, we discussed this a bit before and I think we settled on just overloading Base.getproperty, since that is precisely the mechanism Base provides for overloading getters (as opposed to Base.getfield, the more low-level implementation).
This is also how the abstract State is currently documented:

AlgorithmsInterface.jl/src/interface/state.jl

Lines 11 to 17 in b50c25d

	In order to interact with the stopping criteria, the state should contain the following properties,
	and provide corresponding `getproperty` and `setproperty!` methods.
	* `iteration` – the current iteration step ``k`` that is is currently performed or was last performed
	* `stopping_criterion_state` – a [`StoppingCriterionState`](@ref) that indicates whether an [`Algorithm`](@ref)
	will stop after this iteration or has stopped.
	* `iterate` the current iterate ``x^{(k)}``.

While we can obviously revisit that topic, we should probably not mix that in here. (I also still quite like this decision though)

---

In summary, I now changed the default to just return state.iterate. In the case where you want to keep/reuse the state, this can be achieved by constructing it, and then calling solve! instead of solve.

[kellertuer]

which very quickly becomes painful. (think of dealing with

at least in Manopt that works well (Debug, Record, Return are all states), one mainly has to be strict in accessors being functions. In the end it is a matter of taste and maybe I am just too used (and wired in thought) to using wrappers and doing that carefully – at least I feel it works well in Manopt.

Ah I remember, that we agreed to have the property to be overwritten then, ok

For the solution. you went with, I think that is a good one.

[lkdvos]

I think in that case this PR can probably be merged, and we can go from there. Do you mind approving? I set up some branch protection rules as well in the meantime.

[edit] Unless there were some specific things I still should change, feel free to let me know

[kellertuer]

Surfe, just forgot to do that since it is already 10.30pm here and I just came home form a nice dinner. Here you are.

And sure having those PR approval rules is a good idea!