Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposed stopping criterion tolerances to user #37

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

stefan-k
Copy link
Member

@stefan-k stefan-k commented Apr 6, 2020

This PR makes the tolerances in the stopping criterion of L-BFGS accessible to the user (see #28).

@rth, is this how you imagined it? I would appreciate it if you could give this a quick review.

Copy link
Contributor

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stefan-k ! Sorry I never got to make the PR.

Sounds great, just one comment on naming.

}
}

/// Sets tolerance for precision stopping criterion
pub fn with_tol_precision(mut self, tol_precision: f64) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would help to be more explicit that this tolerance is applied on the gradient. E.g. in scipy they have a pgtol for projected gradient tolerance,

The iteration will stop when max{|proj g_i | i = 1, ..., n} <= pgtol where pg_i is the i-th component of the projected gradient.

How about with_tol_grad? I realize this makes it a bit more difficult to have common API for tolerance across solvers. If you want one main tolerance, maybe with_tol and make it very explicit in the docsting that it's taken on the gradient? I just find the word combination "tolerance precision" confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks. I've opted for with_tol_grad. I've come to think that trying to have a common API across solvers may be counter-productive as tolerances often mean slightly different things for different solvers.

@stefan-k stefan-k merged commit e9239e9 into argmin-rs:master Apr 6, 2020
@stefan-k stefan-k deleted the tol branch April 6, 2020 14:00
@rth
Copy link
Contributor

rth commented Apr 6, 2020

Thanks @stefan-k !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants