search

JuliaManifolds / Manopt.jl

🏔️Manopt. jl – Optimization on Manifolds in Julia
http://manoptjl.org
Other
314 stars 40 forks source link

`:reinitialize_direction_update` for quasi-Newton #388

Closed mateuszbaran closed 3 months ago

mateuszbaran commented 3 months ago

Addresses point 3 from #382. Also a minor cleanup for QuasiNewtonCautiousDirectionUpdate.

kellertuer commented 3 months ago

Looks good at first glance (will take a closer look later); documenter fails, because of caching; it might use the cache from the other PR (of cached packages). Maybe that is a disadvantage of caching packages.

mateuszbaran commented 3 months ago

Are ALM and EPM supposed to keep the state of QuasiNewtonLimitedMemoryDirectionUpdate between runs of the subsolver? I would guess they shouldn't but currently they do.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.76%. Comparing base (d2492a1) to head (bdd1f58).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #388 +/- ## ======================================= Coverage 99.76% 99.76% ======================================= Files 71 71 Lines 7205 7221 +16 ======================================= + Hits 7188 7204 +16 Misses 17 17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kellertuer commented 3 months ago

I think they should not, you are right. Maybe we should have noticed the necessary initialisation there already.

mateuszbaran commented 3 months ago

Caching seems to have fixed itself. By the way, I think :reinitialize_direction_update would be a better default for nondescent_direction_behavior, though it might be considered a breaking change.

kellertuer commented 3 months ago

I think that sounds nice, how breaking would that be? The old one just took a gradient direction right? I would consider that mildly breaking and still ok

mateuszbaran commented 3 months ago

Not that breaking, I will change it if you are OK with it.

kellertuer commented 3 months ago

We also introduced the rule for how to handle non-descent directions as a non-braking improvement, so I am fine with this as well, but please write a detailed change entry about it.

mateuszbaran commented 3 months ago

Is it sufficiently detailed in the latest commit?

kellertuer commented 3 months ago

I prefer to end keywords with = but the rest is exactly the details I was hoping for. Yes this is formerly breaking but I would consider such small improvements in defaults really more like improvements. Yes we do break 100% reproducibility here, but we do not break existing code in the sense that it errors. So I think its fine to keep that nonbreaking