JuliaSmoothOptimizers / ShiftedProximalOperators.jl

Proximal operators for use with RegularizedOptimization
Other
5 stars 7 forks source link

Remove unused delta box operators #99

Closed geoffroyleconte closed 1 year ago

geoffroyleconte commented 1 year ago

For NormL0Box and NormL1Box.

This will be breaking for box operators because shifted(h, x, l, u, Δ) becomes shifted(h, x, l, u), and set_radius! will no longer work (it did not really updated the trust region before but will now return an error message indicating that there is no field Δ, set_bounds! should be used instead). I think that Δ was not used in these shifted operators.

codecov[bot] commented 1 year ago

Codecov Report

Base: 61.79% // Head: 62.00% // Increases project coverage by +0.21% :tada:

Coverage data is based on head (854b51a) compared to base (0b94cfa). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #99 +/- ## ========================================== + Coverage 61.79% 62.00% +0.21% ========================================== Files 22 22 Lines 657 658 +1 ========================================== + Hits 406 408 +2 + Misses 251 250 -1 ``` | [Impacted Files](https://codecov.io/gh/JuliaSmoothOptimizers/ShiftedProximalOperators.jl/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSmoothOptimizers) | Coverage Δ | | |---|---|---| | [src/shiftedNormL0Box.jl](https://codecov.io/gh/JuliaSmoothOptimizers/ShiftedProximalOperators.jl/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSmoothOptimizers#diff-c3JjL3NoaWZ0ZWROb3JtTDBCb3guamw=) | `91.30% <100.00%> (ø)` | | | [src/shiftedNormL1Box.jl](https://codecov.io/gh/JuliaSmoothOptimizers/ShiftedProximalOperators.jl/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSmoothOptimizers#diff-c3JjL3NoaWZ0ZWROb3JtTDFCb3guamw=) | `90.24% <100.00%> (ø)` | | | [src/ShiftedProximalOperators.jl](https://codecov.io/gh/JuliaSmoothOptimizers/ShiftedProximalOperators.jl/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSmoothOptimizers#diff-c3JjL1NoaWZ0ZWRQcm94aW1hbE9wZXJhdG9ycy5qbA==) | `72.22% <0.00%> (+3.65%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSmoothOptimizers). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaSmoothOptimizers)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

dpo commented 1 year ago

If ∆ is not used, it's a bug. Those operators should include the indicator of [ℓ, u] ⋂ ∆𝔹.

geoffroyleconte commented 1 year ago

If ∆ is not used, it's a bug. Those operators should include the indicator of [ℓ, u] ⋂ ∆𝔹.

You used shifted(h, xk, max.(-Δk, l_bound - xk), min.(Δk, u_bound - xk), Δk, selected) in https://github.com/JuliaSmoothOptimizers/RegularizedOptimization.jl/pull/81

Should I understand that you want to use this instead: shifted(h, xk, l_bound - xk, u_bound - xk, Δk, selected)? With the current state of this PR it changes to shifted(h, xk, max.(-Δk, l_bound - xk), min.(Δk, u_bound - xk), selected).

I think the first solution implies calling max internally anyways. And some algorithms don't need $\Delta \mathbb{B}$ (R2 for example). I just want to make sure that it is the right thing to implement before changing everything, and my PR for the iprox is also based on the same design.

dpo commented 1 year ago

You used shifted(h, xk, max.(-Δk, l_bound - xk), min.(Δk, u_bound - xk), Δk, selected)

Ah yes, you're right. The way it is now is right because we can use the same box operators with R2 when there are bound constraints.

dpo commented 1 year ago

thank you