argmin-rs / argmin

Numerical optimization in pure Rust
http://argmin-rs.org
Apache License 2.0
971 stars 76 forks source link

Steepest descent erases prev_param #362

Closed DevonMorris closed 1 year ago

DevonMorris commented 1 year ago

Currently the SteepestDescent solver starts next_iter with state.take_param() setting the current parameter to None. Then once the line search has run, the state is updated with param() which swaps param and prev_param. This in effect leaves the prev_param returned by SteepestDescent via state as None.

This is not a problem for a fixed iteration run of the steepest descent solver, but does become a problem when trying to use SteepestDescent as an inner solver with custom termination criteria based on the previous state.

In more concrete terms, given the current implementation I have to implement something like

        loop {
            // Cache previous inner param for evaluating inner convergence. This is necessary
            // because steepest descent `take_param()` and then updates `param()` effectively
            // erasing the previous value.
            let prev_inner_param = state.param.clone();

            (state, _) = self.inner_solver.next_iter(problem, state)?;
            state.prev_param = prev_inner_param;

            if let TerminationStatus::Terminated(_) = self.terminate(&state) {
                break;
            };
        }

Where inner_solver is a steepest descent. It would be nicer to be able to simply have

        loop {
            (state, _) = self.inner_solver.next_iter(problem, state)?;
            if let TerminationStatus::Terminated(_) = self.terminate(&state) {
                break;
            };
        }
stefan-k commented 1 year ago

Thanks for opening this issue and the helpful analysis! This is a bug: instead of take_param, get_param should be used (followed by cloning the parameters). Would you be willing to open a PR with the fix?

Apologies for the very late reply. My access to internet is currently very limited (#360).

DevonMorris commented 1 year ago

This is a very busy week for me, hopefully I can get around to it next week some time.

stefan-k commented 1 year ago

Sure, take your time! (And I really hope I'll be able to publish a new release soon!)

DevonMorris commented 1 year ago

See https://github.com/argmin-rs/argmin/pull/363/files

DevonMorris commented 1 year ago

Closed by !363