JuliaManifolds / Manopt.jl

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

Checking for non-descent direction in qN #361

Closed mateuszbaran closed 6 months ago

mateuszbaran commented 6 months ago

A slightly generalized check originally proposed in #339 . Ideally we would also have an option that purges the memory but that's fairly difficult to do in Manopt. This change may be a little difficult to test.

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 99.73%. Comparing base (5253f86) to head (92d47b1).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #361 +/- ## ========================================== + Coverage 99.58% 99.73% +0.14% ========================================== Files 73 73 Lines 6801 6805 +4 ========================================== + Hits 6773 6787 +14 + Misses 28 18 -10 ```

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

kellertuer commented 6 months ago

That is a very neat idea. I have to check how to best test that, maybe starting it with -grad for one test?

Also for the warning, I usually did more like message passing and / or have specific debug to catch warnings. I can think a bit what best to do here with that, maybe the warning is also ok.

mateuszbaran commented 6 months ago

I'm trying to add a test by defining a custom struct QuasiNewtonGradientDirectionUpdate <: AbstractQuasiNewtonDirectionUpdate end that returns gradient as the direction.

mateuszbaran commented 6 months ago

I think my only remark would be that this changes the default. (:ignore was the default before), but this could also be seen as an enhancement.

Yes, I think this is an enhancement. It adds a bit of additional cost for checking but I think it's worth it.

We could merge this the next few days together with the typo PR as a new version then :)

Sure, that's fine.