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

Alternative fix to extra allocations in get_gradient! from Jacobian #301

Merged
merged 5 commits into from
Oct 8, 2023

Conversation

mateuszbaran
Copy link
Member

Alternative fix to #300 , as discussed on Slack.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #301 (b3e9c88) into master (48f5ab0) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
- Coverage   99.88%   99.74%   -0.15%     
==========================================
  Files          77       77              
  Lines        7083     7087       +4     
==========================================
- Hits         7075     7069       -6     
- Misses          8       18      +10     
Files Coverage Δ
src/plans/nonlinear_least_squares_plan.jl 100.00% <100.00%> (ø)
src/solvers/LevenbergMarquardt.jl 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kellertuer
Copy link
Member

I fixed the docs, but I am not yet sure what is missing in code coverage by now.

@mateuszbaran
Copy link
Member Author

Thanks for adding docstring to docs! I've added a couple of new tests that may help with coverage loss but some of it looks unrelated to this PR.

@kellertuer
Copy link
Member

Then you have to fix them as well ;)

No the ±10 lines is still something that bothers me since sometimes one of the Wolfe cases is hit, sometimes not and I am not able to consistently provide a test that always hits that. Wolfe is not easy. So 18 lines is still fine due to this.

@mateuszbaran
Copy link
Member Author

OK, I will ignore those 10 lines.

Copy link
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me and we can surely merge that :)

@mateuszbaran
Copy link
Member Author

OK, I will bump version then.

@kellertuer kellertuer merged commit 445e833 into master Oct 8, 2023
12 of 13 checks passed
@Affie
Copy link
Contributor

Affie commented Oct 9, 2023

Thanks for fixing, this takes a big step in making Manopt usable to us.

@mateuszbaran
Copy link
Member Author

You're welcome 🙂 . Let me know if you encounter any other issues with Manifolds.jl or Manopt.jl.

@kellertuer kellertuer deleted the mbaran/gradient-from-jacobian branch November 20, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants