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

Fix PANOCplus #66

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Fix PANOCplus #66

merged 2 commits into from
Feb 18, 2022

Conversation

aldma
Copy link
Collaborator

@aldma aldma commented Feb 17, 2022

This PR fixes a small yet major error:

  • set_next_direction! should use res_prev instead of res, as the latter is tentatively updated.
    The solver with acceleration was working correctly on simple problems only.

Minor change:

  • tau_backtracks is restarted every time a new direction is set.
    Note that the algorithm remains well-defined, namely every iteration terminates, as long as iter.minimum_gamma > 0.

@aldma aldma changed the title Fix panocplus Fix PANOCplus Feb 17, 2022
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #66 (a9f159e) into master (52ef0f0) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   88.59%   88.57%   -0.02%     
==========================================
  Files          22       22              
  Lines         894      893       -1     
==========================================
- Hits          792      791       -1     
  Misses        102      102              
Impacted Files Coverage Δ
src/algorithms/panocplus.jl 93.50% <83.33%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52ef0f0...a9f159e. Read the comment docs.

@@ -178,7 +177,6 @@ function Base.iterate(iter::PANOCplusIteration{R}, state::PANOCplusState) where
break
end
state.tau = tau_backtracks >= iter.max_backtracks - 1 ? R(0) : state.tau / 2
tau_backtracks += 1
Copy link
Member

@lostella lostella Feb 17, 2022

Choose a reason for hiding this comment

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

This value is used in the lines immediately above this one, in some checks: if the value is instead updated earlier (at the top of the loop) probably the inequalities involving it need to be adjusted? (In order to maintain the exact same behavior, that is)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The algorithm is identical in this regard, since it starts always with can_update_direction = true and so it takes tau_backtracks = 0. Then the update tau_backtracks += 1, or the reset, is effectively performed after the checks you mention.

@lostella lostella merged commit 69e6fc2 into JuliaFirstOrder:master Feb 18, 2022
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