Added OptimResult #10
Conversation
| /// The number of function evaluations performed. | ||
| pub f_evals: Option<usize>, | ||
| /// The number of iterations run. | ||
| pub iterations: Option<usize>, |
There was a problem hiding this comment.
It is not clear to me why runtime,f_evals and iterations are Options. Are there cases imaginable where these don't have 'default' values? I would expect every algorithm can return appropriate values for these fields.
There was a problem hiding this comment.
From my point of view, two reasons:
- I did not bother considering all usecases for each parameter.
- I want a uniform interface for all of them anyway. I think the additional costs don't matter as much, but it simply adds more flexibility.
|
|
||
| /// Minimizer states at the end of the run | ||
| #[derive(Debug, PartialEq)] | ||
| pub enum RunStatus { |
There was a problem hiding this comment.
The meaning of RunStatus is not entirely clear to me. I assume this enum should reflect the reason for termination, but then it should differentiate between e.g. ftol convergence, xtol convergence or maxiter exceeded. See also my comment in the Nelder-Mead code
There was a problem hiding this comment.
Agreed, will change this. The PR is only a WIP for now anyway. I was thinking of adding more states, such as warmup, inprogress, one for various convergences. This can also be kept in the minimizer state (and updated via something like is_finished in NM). Maybe at some point it can also be potentially used to save the state of an in-progress minimization to restart or continue.
| iterations: Some(iterations), | ||
| minimum: Some(MinResult::Vector(best.1)), | ||
| minimum_value: Some(best.0), | ||
| status: RunStatus::Success, |
There was a problem hiding this comment.
Shouldn't the status be tied to some conditionality? This relates to my other comment about the meaning of this enum.
|
Regarding the fmt, you can add |
|
Just for keeping track of thoughts:
|
Implements #9
Also ran
cargo fmton the crate... Will attempt to add this as a requirement for every PR@sebasv if you could have a look if you agree with the implementation?