JuliaManifolds / Manopt.jl

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

Improve Linesearch and Quasi Newton allocations #335

Closed kellertuer closed 8 months ago

kellertuer commented 9 months ago

This PR is work towards having some memory with line searches to reuse candidate (point) memory.

This resolves parts of #333, namely for now

ToDo

kellertuer commented 9 months ago

Main thing to fix: Circle manifold

Minor thing to fix – check whether we can avoid the eta allocation from (4).

mateuszbaran commented 9 months ago

I've fixed the linesearch error for Circle but not it does not converge for some reason :confused:

codecov[bot] commented 9 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (0bca9b5) 99.65% compared to head (525d4f7) 99.63%.

Files Patch % Lines
src/plans/stepsize.jl 88.67% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #335 +/- ## ========================================== - Coverage 99.65% 99.63% -0.02% ========================================== Files 69 69 Lines 6342 6361 +19 ========================================== + Hits 6320 6338 +18 - Misses 22 23 +1 ```

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

mateuszbaran commented 9 months ago

I need to fix the failure on 1.6 in Manifolds.jl.

mateuszbaran commented 9 months ago

Done here: https://github.com/JuliaManifolds/Manifolds.jl/pull/692/commits/5fa4eed08f6abd8e73599b7cc0d1f8dac53cb740 .

kellertuer commented 9 months ago

Okay then let's wait for that one.

We are missing code coverage here anyways probably; and I would like to check the docs the next few days calmly again.

kellertuer commented 9 months ago

Oh and you found a way for the qN update – super neat!

mateuszbaran commented 9 months ago

It was just a minor bug :slightly_smiling_face:

kellertuer commented 9 months ago

Now just test coverage is missing for the (old, allocating) Quasi Newton Updates. And of course if we see a good way to remove the 10 missing lines from WolfePowell that would be great, it is one of the few places where I was not yet able to hit that case in tests.

kellertuer commented 9 months ago

I got the overall coverage to be fine again; but the patch one is not fulfilled since we now have a few more lines in the uncovered case in WolfePowell

kellertuer commented 8 months ago

@mateuszbaran can you check the fail on 1.6? I though we fixed that for circle with the PR for Manifolds.jl 0.9.9...

mateuszbaran commented 8 months ago

We've fixed another, similar issue but not this one... I will fix this.

kellertuer commented 8 months ago

Oh, then I confused that, but thanks for fixing this so fast again :)

kellertuer commented 8 months ago

Hm, I am not aware we did breaking changes in Manifolds is ManifoldsBase but it seems the same thing you just fixed on Circle also happens on Euclidean.

mateuszbaran commented 8 months ago

IIRC it's not breaking, it's just a bug that was exposed by these changes. I will fix that too.

mateuszbaran commented 8 months ago

(exposed by changes in this PR)

kellertuer commented 8 months ago

Thanks!

Suer fixing bugs is nice, and they all seem to be the same area, but it is annoying to check for ;)

mateuszbaran commented 8 months ago

It's a really weird case I'd probably dismiss with "don't do that" if it didn't come up as a test failure here :sweat_smile:

kellertuer commented 8 months ago

The increase in code coverage is unfortunate but the one uncovered block this project has is now a few lines longer. So this should still be fine.