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

Adding time limit to levenberg_marquardt.jl #204

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Gkreindler
Copy link

This small edit allows levenberg_marquardt() to exit if a specified time limit is exceeded.

This is my first ever pull request, so any tips or pointers to best practices are welcome!

@Gkreindler
Copy link
Author

Hello, any change someone might be able to review this? I think it is very simple, and I would like to use this feature in a package that I am working on.
Thank you in advance!

src/levenberg_marquardt.jl Outdated Show resolved Hide resolved
src/levenberg_marquardt.jl Outdated Show resolved Hide resolved
src/levenberg_marquardt.jl Outdated Show resolved Hide resolved
@pkofod
Copy link
Member

pkofod commented Sep 2, 2022

This is my first ever pull request, so any tips or pointers to best practices are welcome!

I'm sorry for not reviewing, I appreciate the effort! I changed your initial value from -1 to NaN. I don't think it makes sense to restrict it to integers as time() returns a Float64 value. By setting it to NaN we can be sure that time() - t0 < time_limit is never true, because no number is less (or larger) than NaN.

The other comment I have is that typically you'd be required to add a test. Sometimes it's a bit hard to come up with a test. In this case you could maybe use a model that would otherwise converge fine, but that has sleep(10) in it and set the time limit to 1 such that you know it will be violated on the very first iteration. Then, you can test that the number of iterations was indeed 1, and then afterwards test the same model without the sleep and verify that it did indeed converge fine without a time limit.

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