[lkdvos]

This PR reduces the boilerplate needed to implement an algorithm.

• DefaultState <: State stores the expected iterate, iteration and stopping_criterion_state, plus a data field for any additional data.
• Default initialize_state and initialize_state! methods construct and reset a DefaultState, taking the initial point as the iterate keyword (or positionally as (problem, algorithm, iterate, state_data)).
• Default initialize_state and initialize_state! for any StoppingCriterion, using a DefaultStoppingCriterionState. StopAfterIteration now relies on these.

With this, an algorithm only has to provide a Problem, an Algorithm and a step!.
The abstract State stays, so algorithms that need more can still define their own state, as before. This change is non-breaking.

The docs now use DefaultState for the Heron example, and show a custom HeronState at the end of the interface page.

[lkdvos]

@mtfishman, @jack-dunham - linking you both here in case you would like to have a look/share opinions

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (b3e5de7) to head (963aa75).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
+ Coverage   99.60%   99.62%   +0.02%     
==========================================
  Files           6        7       +1     
  Lines         255      270      +15     
==========================================
+ Hits          254      269      +15     
  Misses          1        1              

☔ View full report in Codecov by Harness.
📢 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]

I like the idea, though until now I mainly used a slightly different approach (in Manifolds.jl and Manopt.jl that is):

the DefaultState would illustrate the minimal things one needs, but not store data.
Any algorithm that needs something additional would repeat the 3 fields in its state and then add its own fields.

I am not saying my approach is per se better. I do see the danger of “overwrapping” in your case though. Somewhen you might end up with “DefaultData” and therein continue with some further default...

What do you think?

Another note, since we are discussing states, in Manopt.jl we introduced callbacks a while back, see JuliaManifolds/Manopt.jl#626 or https://manoptjl.org/stable/tutorials/HowToCallback/ for the new tutorial. Since we do talk about a default state, we could add that here as well, indeed. The field is just a dictionary of functions (of an agreed-upon-signature), the actual callback code is quite short https://github.com/JuliaManifolds/Manopt.jl/blob/master/src/base/state/callback.jl and the actually “hooks” (places where the callbacks are called) are also just one line each, see for example the generic ones at https://github.com/JuliaManifolds/Manopt.jl/blob/5e362ee0f07fb656dfdf1a40a616a94c3ba27260/src/solvers/solver.jl#L164-L177

[lkdvos]

I like the idea, though until now I mainly used a slightly different approach (in Manifolds.jl and Manopt.jl that is):

the DefaultState would illustrate the minimal things one needs, but not store data.
Any algorithm that needs something additional would repeat the 3 fields in its state and then add its own fields.

I am not saying my approach is per se better. I do see the danger of “overwrapping” in your case though. Somewhen you might end up with “DefaultData” and therein continue with some further default...

I definitely feel like I agree with you here, I think this was what I was getting at with the question about replacing the abstract State directly with the current implementation. I think I felt the things you are discussing but didn't really realize what was causing it :)

Let me try that out just to see how it would look, the only annoying part being that it is breaking but that's what 0.x versions are for I guess.

[kellertuer]

I still think the DefaultState is a very good idea. Sometimes an iterate and a a stopping criterion is all that is basically needed. Otherwise it still serves as a great reference what a state should contain :)

[lkdvos]

Alright, second attempt:

I took the State thing seriously now and moved a bunch of stuff around, I think overall this is definitely an improvement but I'd be interested to hear other opinions on this.

What I like the most about this approach is that we no longer rely on correctly defining the right fields for compliance with this package, and in the meantime get to hide a lot of the boilerplate that is related to that.

What I like a little less, although I think it might be acceptable, is that because we are defining the structs, it is a little less transparent what is going on and which data is put where. Ultimately, I think I quite like this approach.

[mtfishman]

I like the idea of having a DefaultState, though I think I agree with @kellertuer that a downside of the latest design is that if you want to dispatch you then need a struct you put in data so it pushes the complexity there. I.e. I feel like if you want a custom state object (say, you want to be able to write state.x = ... for a custom field x) it would be easier to define a new State subtype rather than define a new type to go in data and then dispatch on that (obviously both approaches can achieve the same thing but the mental burden of making a new data object feels higher to me).

[kellertuer]

In the last commit the abstract State was removed? I think this is still very relevant and important to have, there might be more complex states to have in mit than the (Default)State we now have.

[lkdvos]

This last commit was the idea to get rid of the abstract state, so the default path is without as much friction, and then have explicit implementations add a .data field they can use. One thing that might be useful there is that in principle I can make that a NamedTuple and forward the getproperty calls, which effectively turns the whole thing into a dynamic struct that can hold any data and fields you want, but is guaranteed to have the ones we need. I agree that for dispatch this is less suited, but the more I think about this the more I feel like probably a single algorithm/problem combination should be tied to a single state anyways, to avoid nightmares with ambiguity issues.

[jack-dunham]

The DefaultState idea is good, although my preference would personally be to remove the abstract State (as you have done), but also remove signatures that have ::State as an annotation, in favour of ::Any. The interface for a state object (which can subtype anything it likes) is then simply:

state.iterate     # returns the iterate (the data that gets passed around)
state.iteration   # returns the iteration. 

Then one can define concrete DefaultState with the data field for convenience, and overload the associated methods to also accommodate state.data on DefaultState.

[kellertuer]

I was maybe too much still in Manopt and thought every algorithm would need its own state. That is not the case, though different algorithms need very different “dynamic” parts they store in the state.

The “dynamic structure with names tuple” sounds a bit like a hack. For me it then depends a lot on how “safe” that is, whether I like that. I am not yet sure.

If we always one have one state, then sure we do not need a default one.

[lkdvos]

Found some more time for this one:

From the discussion before (at least what I remember from them), I think I went more or less back to the first commit and kept the abstract State and StoppingCriterionState, making this non-breaking again.

we then still have:

• DefaultState <: State: the no-boilerplate path, with a single opaque data field that can be used if needed
• DefaultStoppingCriterionState remains as is, no data field but counts as the generic fallback for criteria that don't need initialization

I removed the getproperty forwarding to discourage the NamedTuple-style of abusing DefaultState, since this now keeps the abstract State so if you require fancy state types you should probably just define them.

[kellertuer]

Ehm, the commit message is AI slurp – the docs now also tend to that, is that intended?

[kellertuer]

also very long and slurp. No need to write that there is no good default.

[lkdvos]

Fair point(s), I squashed the commits now, and went through to make the language a bit better (hopefully).

Re the individual commit messages, I don't care too much about that, since they end up squashed in the main repository anyway, and I'm not that convinced we're gaining much by me investing time in cleaning that up...

Hopefully this now looks clean and acceptable to everyone :)

[kellertuer]

Let me then start with a bit of a polemic answer quote

If you do not care to write something, why should I care to read it?

I have given up discussing LLMs in e.g. Julia Discourse, there is too many evangelist and vibe coders around.
I am not rich enough to afford a subscription myself. I have support from the Julia Community currently to use one and I am trying to improve the code base of packages I maintain. The LLM finds bugs. It even fixes typos and sign errors in formulae – but the coding and “new docs” it proposes are horrible. Hard to read, duplicate code, look for the easiest way out to patch things.

This package has turned into something I will probably never use in practice. I still think it is nice, but I would estimate a full year of work to base Manopt on this. And I will never have such a year.
So while Manopt.jl is already mainly a hobby (never got any funding or recognition whatsoever for it, probably nearly no one even uses it, visibility 0) and that is fine – this is a hobby of a hobby.
Do I want to get also AI slopped in my free time? Do I want to have any LLM as a commit author in my hobbies?

No, I do not want that.

This does not mean you have to stop using it. It does also not mean, you have to stop using it here. Just if you want to use it here, I am no longer available as reviewer or for discussions. This is nothing against you personally – I think we had absolutely great discussions. I am just too overly tired of LLMs and being “slopped all around”.

[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.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

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