Refactoring convergence criteria

[MaxvdKolk]

Not to be merged yet, but mostly as a point to get some feedback on possible changes to the convergence criterion. I had the impression that some parts could be made more concise, see the changes in this PR. There are some points to think about still,

• For the KKT conditions, multiple variables need to be request/passed through the criterion call. As most others simply just have one variable that is required to evaluate the criteria, it might make sense to provide a separate function somewhere in the code that evaluates the KKT conditions. In that case, it could be come like the following

    while not criteria.converged: 
         ... 
         criteria(kkt=ktt_condition(...), ... )

• There is still some overlap between some of the criteria and I wonder if it can be made more general to prevent that.

[artofscience]

Max and I have the following concept:

First create a storage class, something like:

class Storage(solver, subproblem, problem etc.) self.solver = solver self.subproblem = subproblem self.problem = problem

The usage of this class is multiple:

1. We can use it too simplify the criteria evaluation (all info is available)
2. We can use it for plotting, logging, saving data, and maybe global convergence
3. Possibly we can generate a Report() function, that prints a text file with all constants etc. Preferably we want to be able (for each class in the framework), to give a flag or so to determine which constants should be stored. (maybe something for later) This would add a lot to replication of results. Even nicer would be if one can read in the report and run it.

Next: build some criterion (multiple possibly), e.g.

loop = 0

mykkt = kkt(tol=1e-4) myfeasibility = feasibility(slack=1e-9) myloop = iter(loop, max=500) myiter = iter(iter, max= 10)

Those all inherit from an abstract class Criterion:

• and(self, other) -> Criterion
• or(self, other) -> Criterion
• bool(self) -> true/false

Then build the convergence class (or possibly simply an lambda function..), e.g.

myconvergence = (c1 | c2) | c3 # & iter < 500

while not myconvergence # & iter < 500 : loop = loop + 1 for abc iter = iter + 1

for it in range(maxiter): if converged: break ...

[MaxvdKolk]

I'll give it a go to implement the above proposal, so don't spend time review the previous 2 commits :-)

[MaxvdKolk]

The added commit attempts to implement (part of) the features as proposed in the concept mentioned above. Most specifically, it should allow for combining criteria using & and |, for instance for both objective change and feasibility:

prob = Square(...)
obj_change = ObjectiveChange(prob, tol=1e-4)
feasibility = Feasibility(prob, slack=1e-9)
converged = obj_change & feasiblity

while not converged:
    ...

To add a maximum iteration count, only change that is required:

converged = (obj_change & feasibility) | IterationCount(100)

Should also be possible to invert the criteria, e.g. to request a minimal number of iterations

converged = obj_change & feasibility & ~IterationCount(10)

Here ~IterationCount(10) will only return True once 10 iterations are passed.

Please take a look at sao/convergence_criteria/criteria.py for the details. I still need to add the proper Storage class, but this seems to be a small/minimal implementation to illustrate the behaviour.

[Giannis1993]

Max and I have the following concept:

First create a storage class, something like:

class Storage(solver, subproblem, problem etc.) self.solver = solver self.subproblem = subproblem self.problem = problem

Do we want to force the user to create an instance of the Problem class? Cuz I remember we said that users might also just want to import their g and dg as np arrays.

The idea of a Storage class is very nice though and might simplify logging / plotting / saving data, since (more often than not) users are going to log / plot / save convergence related data (e.g. KKT residual, norm of design change, objective, etc.).

[MaxvdKolk]

Still to add:

• KKT
• Variable change
• Update all examples using criteria to use the updated implementation

[MaxvdKolk]

Included the comment by @artofscience and added an initial VariableChange implementation. Still to be done:

• a basic storage class
• the KKT criterion

[artofscience]

@MaxvdKolk @Giannis1993 @aatmdelissen I had a discussion with Arnoud, who argumented a storage class will hurt the modularity of the code. Can we get around this?

[MaxvdKolk]

Sure, just pass the arguments individually. Not sure if it really degrades the modularity though, it just wraps together the different instances to pass around. Of course, we can simply pass whatever is needed to the convergence class itself, either as a tuple, or separate arguments. There is not really a difference.

---

Still need to look to updating this, but have had little time recently, so feel free to propose something else completely, if that is easier and moves the code forward.

[Giannis1993]

I think I agree with @MaxvdKolk. Whether we pass them individually to the convergence class, or we first wrap them in a storage class and then pass them I don't see what the difference is (in terms of modularity at least). Maybe avoiding the storage class would make it easier to understand (since less objects are involved) but maybe the storage class would simplify sharing information.

---

I would let the storage class find its place and it will show if its adds any value or not.

[artofscience]

Still need to look to updating this, but have had little time recently, so feel free to propose something else completely, if that is easier and moves the code forward.

Imo best procedure would be to implement the KKT (just by passing f,g,lambda) and then close this PR. A storage class would become a new PR. Let me know if I can help finishing the PR

[MaxvdKolk]

Still need to look to updating this, but have had little time recently, so feel free to propose something else completely, if that is easier and moves the code forward.

Imo best procedure would be to implement the KKT (just by passing f,g,lambda) and then close this PR. A storage class would become a new PR. Let me know if I can help finishing the PR

Fully agree. I will update this branch to be on top of main again, remove the Storage class and update the code accordingly. Also, will try to improve the tests for the criteria.

[MaxvdKolk]

Current status:

• Branch is up to date with latest changes in main
• Contains small tests for the different Criterion implementations, although should be added to examples as well
• KKT: to be done

[artofscience]

@MaxvdKolk could you check what are the conflicts of this PR? If you can resolve them we can merge this PR and add the KKT later.

[MaxvdKolk]

@artofscience The conflicts are resolved. There are some small issues though, in examples/ where still the old criteria are used to pass to the plotter etc. Might have time to look into that later this week. The tests are working though.