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

Drop cppoptlib #66

Merged
merged 14 commits into from
Feb 9, 2024
Merged

Drop cppoptlib #66

merged 14 commits into from
Feb 9, 2024

Conversation

zfergus
Copy link
Member

@zfergus zfergus commented Jan 5, 2024

I did this, but I have mixed feelings about it.

Pros:

Cons:

  • It ended up being almost a copy-paste from cppoptlib (older version) as we use a lot of features in Problem but little in Solver
  • It still requires cppoptlib for two line-search methods: CppOptArmijo and MoreThuente

We can talk about whether to merge these changes here. If not, we can keep the improved comments in Problem.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

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

Comparison is base (811c9a7) 89.00% compared to head (ca0611e) 87.27%.

Files Patch % Lines
src/polysolve/nonlinear/Criteria.cpp 35.71% 45 Missing ⚠️
src/polysolve/nonlinear/Solver.cpp 91.35% 7 Missing ⚠️
src/polysolve/nonlinear/Problem.hpp 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
- Coverage   89.00%   87.27%   -1.73%     
==========================================
  Files          51       48       -3     
  Lines        2001     2028      +27     
==========================================
- Hits         1781     1770      -11     
- Misses        220      258      +38     
Flag Coverage Δ
polysolve 87.27% <68.23%> (-1.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@teseoch
Copy link
Member

teseoch commented Jan 30, 2024

I never liked the criteria class, because we couldn't extend it. To me, that is the only thing you copied. I would clean and refactor it a bit, so in the end, it is not a copy paste.

Regarding the two LS strategies, we can do without or integrate them in the new framework. Note that they don't properly work with the step valid stuff

src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Criteria.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Solver.hpp Outdated Show resolved Hide resolved
src/polysolve/nonlinear/Solver.hpp Outdated Show resolved Hide resolved
@zfergus zfergus marked this pull request as ready for review February 8, 2024 23:16
@zfergus zfergus changed the title Drop cppoptlib from Problem and Solver Drop cppoptlib Feb 9, 2024
@zfergus zfergus merged commit d65d1b1 into main Feb 9, 2024
7 of 9 checks passed
@zfergus zfergus deleted the drop-cppoptlib branch February 9, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment