Use builder pattern to simplify the API

[bytesnake]

Hey, thanks for creating and maintaining this crate :) I only glanced over the examples, but wondered why you don't use the builder pattern. It could simplify setting up a new solver a bit, otherwise the crate is looking really useful. Greetings from Aachen.

[stefan-k]

Hi, thanks for this comment. I was actually considering this a while ago but soon gave up, secretly hoping that someone will suggest this and therefore force me to think about it again in detail. So, thanks, I guess ;)
I do really like the builder pattern, but I see some problems with it, namely because I think the design of this crate should follow two principles:

1. Because of future plans I have, the solvers should be object safe (meaning that none of the methods in the ArgminSolver trait can return Self).

2. It should be completely transparent to the user whether a method is part of the ArgminSolver trait or is part of impl MySolver { ... }.

Currently, a solver is implemented like this (very, very simplified):

#[derive(ArgminSolver)]
struct MySolver {
    variable: f64,
    base: ArgminBase,
}

impl MySolver {
    pub fn new() -> Self {
        MySolver {
            variable: 1.0,
            base: ArgminBase::new(),
        }
    }

    pub fn set_variable(&mut self, var: f64) {
        self.variable = var;
    }
}

impl ArgminNextIter for MySolver {
    // Not important for the purpose of this example
}

The #[derive(ArgminSolver)] macro (which is currently rather stupid) will create something like this:

impl ArgminSolver for MySolver {
     // Associated types and a bunch of methods

    // example method
    fn set_max_iters(iters: u64) { 
        self.base.set_max_iters(iters);
    }
}

I have difficulty seeing where the builder should come in. If there were a MySolverBuilder, then the user would first have to call set_variable on the builder, build it (in order to get a MySolver), and then call set_max_iters(). From the point of view of a user, this makes no sense and it violates principle number 2 from above (and it isn't really a builder pattern anymore).
Currently, all the setters (which essentially act on the base field of MySolver) are part of the ArgminSolver trait. If those should follow a builder pattern, they would need to return Self and therefore could not be part of ArgminSolver (because of object safety (principle number 1)), but instead would need to belong to another (not yet existing) trait (ArgminSolverBuilder perhaps?). This is possible, and probably the most viable way, but then there would be two builders, one implemented via the ArgminSolverBuilder trait (which could be derived automatically) and one by the struct MySolverBuilder. I'm not sure how they'd work together, but I suppose it would work. It would certainly solve some of the ugliness in some of the line searches, but in general I'm not sure whether it is worth the effort.

I hope this wasn't too confusing. I'm happy to give my explanations another try if they are incomprehensible ;)
What do you think about this? Do you have any other suggestions/views?
Would you be interested in giving this a shot in a PR? This would be quite a deep dive into the codebase, but I'd obviously help as much as I can.

Greetings from the south ;)

[bytesnake]

This seems indeed very difficult, perhaps yet another option would be to split each solver into a struct which executes the optimization (i.e. the ArgMinBase struct) and the actual routine with its parameters. This has the advantage that you won't need the derivation crate to emulate classes and you can implement separate builder with different return types. Obviously the API surface is more complex because you have to use at least to struct to initiate an optimization process.

...
let solver = BacktrackingLineSearch::new(operator, cond)
   .set_search_direction(vec![-2.0, 0.0])
   .set_rho(0.9)?
   ...;

let executor = Framework::new()
    .with_solver(solver)
    .max_iters(1000)
    .add_logger(ArgminSlogLogger::term())
    .run()?;

I don't know which one is more sensible, but for now I have no time because of exam time, perhaps in the middle of march :)

[stefan-k]

This is an excellent suggestion, thanks a lot! This would solve many problems, in particular it would remove the ugly base field and I suppose it will solve a problem I'm having with trait bounds. I don't mind having two structs.
I'll probably give this a try this weekend.

Good luck with your exams! :)

[stefan-k]

When I implemented the codegen crate I was well aware that I'm doing this mostly for vanity reasons and for my ego, but I thought it was serving at least some purpose. It wasn't. While implementing your suggestion it became clear that the old design made everything much more complicated than necessary.

Over the last week I've changed the design completely and simplified other parts along the way. Overall many problems I've had with the previous design vanished with the new design. It is still not entirely done but I think the major parts work. Therefore I've merged it into master. I will continue improving the design and documentation over the next days and I will reimplement the tests for the new design.

Thanks once again for your suggestion! If you have time and if you are interested, I'd appreciate it if you would contribute! :)

[bytesnake]

Glad I could help :)