Skip to content

Small Fix #120

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

Merged
merged 3 commits into from
Sep 15, 2024
Merged

Small Fix #120

merged 3 commits into from
Sep 15, 2024

Conversation

nrummel
Copy link
Contributor

@nrummel nrummel commented Jul 23, 2024

Hello JSO Team,

I wanted to use your ARCqK algorithm in my work, but it was cycling when my cost function was misbehaving. I think the algorithm is supposed to exit when there are too many unsuccessful iterations in a row, but instead it got stuck in an infinite loop until time controls hit.
Also, I want to pass the nlp_at_x the callback so that I have access to the current x at each iteration.
Finally, I think the counter for very successful iterations was in the wrong place.

Thanks for making your algorithm available in Julia.
Nic

Bug Fix: exit if too many unsuccessful iterations in a row
@tmigot
Copy link
Member

tmigot commented Jul 24, 2024

Hi @nrummel ! Thanks for the PR, I think this is connect to this issue: #84

@tmigot tmigot linked an issue Jul 24, 2024 that may be closed by this pull request
Copy link
Member

@tmigot tmigot left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nrummel ! There are still improvements that can be made to this algorithm, so thank you for testing and contributing.

I made some comments, but overall it looks good.

@@ -148,8 +148,8 @@ function SolverCore.solve!(
[Int64, T, T, T, String, T, T, T],
)
verbose > 0 && @info log_row(Any[iter, ft, norm_∇f, 0.0, "First iteration", α])

callback(nlp, solver, stats)
Copy link
Member

Choose a reason for hiding this comment

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

You can actually access the nlp_at_x by solver.stp.nlp_at_x , so I prefer callback(nlp, solver, stats)

@nrummel
Copy link
Contributor Author

nrummel commented Aug 28, 2024

I still am not seeing the remote/main having these changes. It would be nice if this was merged into main, so I don't have to dev this repo anymore. Thanks again. Nic

@tmigot
Copy link
Member

tmigot commented Sep 1, 2024

Hi @nrummel ! As you can see here, I made some comments that you need to adress before the PR is being merged.

@tmigot tmigot merged commit f9e6d2c into JuliaSmoothOptimizers:main Sep 15, 2024
7 of 10 checks passed
@nrummel
Copy link
Contributor Author

nrummel commented Sep 17, 2024

Thanks for doing this. Apologies, I have been pushing this small task down my to do list for a while now.

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.

Infinite loop if only unsuccessful steps
2 participants