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

Improve Linesearch and Quasi Newton allocations #335

Merged
merged 23 commits into from
Dec 28, 2023

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Dec 16, 2023

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

This resolves parts of #333, namely for now

  • 6 (in linesearch_ backtrack which now has a variant linesearch_ backtrack!)
  • 1, 2, 5 and a mother temporary storage for the direction (formerly \eta_new)

ToDo

  • Currently this PR breaks for example CG on the Circle (or Armijo/Wolfe on the circle).
  • quasi Newton might now have a copy too less since some side effect now lead to NaN results in the tests – when we have new memory for eta, it works fine for now, but we might want to check where to fix that best otherwise.
  • Changelog entry
  • Maybe since we are revising step sizes – they are currently very Float64-focussed, but I was not yet able to allow other real types therein

src/plans/stepsize.jl Outdated Show resolved Hide resolved
src/plans/stepsize.jl Outdated Show resolved Hide resolved
src/plans/stepsize.jl Outdated Show resolved Hide resolved
src/plans/stepsize.jl Outdated Show resolved Hide resolved
src/plans/stepsize.jl Outdated Show resolved Hide resolved
test/plans/test_stepsize.jl Outdated Show resolved Hide resolved
@kellertuer kellertuer changed the title Inpace candidates in LInesearch Inplace candidates in LInesearch Dec 16, 2023
@kellertuer
Copy link
Member Author

Main thing to fix: Circle manifold

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

@kellertuer kellertuer marked this pull request as ready for review December 16, 2023 13:02
@kellertuer kellertuer changed the title Inplace candidates in LInesearch Improve Linesearch and Quasi Newton allocations Dec 16, 2023
@mateuszbaran
Copy link
Member

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

kellertuer and others added 2 commits December 16, 2023 14:38
…liaManifolds/Manopt.jl into kellertuer/fix-linesearch-allocations
src/plans/stepsize.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 16, 2023

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mateuszbaran
Copy link
Member

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

@mateuszbaran
Copy link
Member

Done here: JuliaManifolds/Manifolds.jl@5fa4eed .

@kellertuer
Copy link
Member Author

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
Copy link
Member Author

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

@mateuszbaran
Copy link
Member

It was just a minor bug 🙂

@kellertuer
Copy link
Member Author

kellertuer commented Dec 17, 2023

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
Copy link
Member Author

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 kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Dec 18, 2023
@kellertuer
Copy link
Member Author

@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
Copy link
Member

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

@kellertuer
Copy link
Member Author

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

@kellertuer
Copy link
Member Author

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
Copy link
Member

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

@mateuszbaran
Copy link
Member

(exposed by changes in this PR)

@kellertuer
Copy link
Member Author

Thanks!

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

@mateuszbaran
Copy link
Member

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 😅

@kellertuer
Copy link
Member Author

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.

@kellertuer kellertuer merged commit aef51e5 into master Dec 28, 2023
13 of 15 checks passed
@kellertuer kellertuer deleted the kellertuer/fix-linesearch-allocations branch May 4, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants