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.

Sign up for GitHub

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

Jump to bottom

Minpack rewrite #8

Merged
merged 73 commits into from
Apr 30, 2020
Merged

Minpack rewrite #8

merged 73 commits into from
Apr 30, 2020

Conversation

jannschu
Copy link
Contributor

Closes #2, #3, #4 and #6.

I am quite confident that the implementation is correct now.

@jannschu
Rework API to use a optimization target trait
b3c27e8
@jannschu
Split parts up into separate files
bac1cd6
@jannschu
Implement pivoted QR and LLS solving
f81df34
@jannschu
Sketch of trust-region sub problem
2022e3f
@jannschu
Fix QR and implement trust-region subproblem
d9a6d78
@jannschu
Change LLS storage format for l_diag
e5d3287
@jannschu
Finish first draft of LM implementation
02f9b13
@jannschu
Remove DimName bound
8bc3c93
@jannschu
Fix the line fitting test
7334253 
Use other parametrization for normal. The derivatives were wrong after normalization.
@jannschu
Add missing sqrt for the residual norms
c722d99
@jannschu
Decrease accepted error by 10x in line fitting test
e97423c
@jannschu
Remove API for parameter normalization
25ae9f5 
This is likely to cause wrong derivatives. If you want constrained
optimization use a different algorithm or parametrization.
@jannschu
Add simple finite difference utility function
b04aeda
@jannschu
Use approx crate for comparing floats in the tests
aa59393
@jannschu
Tweak documentation
04b8325
@jannschu
Split LM algorithm into smaller functions
418b92d
@jannschu
Update authors
2074d67
@jannschu
Add unit tests for "init" and "update_diag" step of LM algorithm
9f375ba
@jannschu
Fix gtol check for nan or inf values
1a043e7
@jannschu
Fix sign error in numerical differentiation utils
b1e2445
@jannschu
Tweak documentation
acca5c6
@jannschu
Fix gtol < eps test
6d8df9b
@jannschu
README and docs
5fc8cc1
@jannschu
Bump version number to 0.5
62484aa
@jannschu
Try to get exactly the same FP output as MINPACK
fc22a4c
@jannschu
Call set_params with the final params
d55c7e3
@jannschu
More fixes for FP equality with MINPACK
badc899
@jannschu
Fix clippy warning
c46a11c
@jannschu
Add linear test function tests
eb1a3b0
@jannschu
Add un/likely for norm function
1b33fe5
jannschu and others added 15 commits April 27, 2020 10:49
@jannschu
Add generator for the Rust code MINPACK tests
4367570
@jannschu
Fix no_std for nightly
a12d94d
@jannschu
Tweak documentation for numerical differentiation
2e18dbf
@jannschu
Fix whitespace
0564716
@jannschu
Try to make the meyer test pass on macOS and linux
63a31b4
@jannschu
Fix generated tests for no minpack-compat mode
3115747
@jannschu
Decrease optimization level for tests
00369c2
@jannschu
Fix typo for minpack-compat feature check in tests
217448d
@vadixidav
Revert "add TODO about minpack compat mode in tests"
e1f7849 
This reverts commit cd42bd7.
@vadixidav
Revert "fix non-minpack compat test cases by adding optional epsilon"
d6dcc3d 
This reverts commit 8d43f3d.
@jannschu
Merge pull request #1 from vadixidav/minpack-rewrite-relative-eq-fixes
e10deb8
@jannschu
Add runtime check to ensure correct dimensions
bb7e588 
When using `Dynamic` the returned residuals or Jacobians could
change. This is an error.
@jannschu
Fix typo
bde32fd
@jannschu
Remove unused trait bounds from LeastSquaresProblem
41afe7b
@jannschu
Enable KaTeX rendering for comments
56d5416
@jannschu
Copy link
Contributor Author

jannschu commented Apr 29, 2020 •
edited
Loading

I will now work on removing the restriction n <= m (number of residuals can not be less than number of parameters).

But this restriction does not block a release.

vadixidav
vadixidav reviewed Apr 29, 2020
View reviewed changes
Copy link
Member

@vadixidav vadixidav left a comment

Choose a reason for hiding this comment

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

Awesome! You have done an immense amount of work, and I appreciate it so much. This is one of the most important algorithms for us to have.

Any of the changes in the review I can implement and set up a PR to your branch. I would just like your feedback on whether these are good ideas.

src/problem.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/lm.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lm.rs Outdated Show resolved Hide resolved
src/problem.rs Outdated Show resolved Hide resolved
src/problem.rs Show resolved Hide resolved
@jannschu
Copy link
Contributor Author

Ok, I adressed the review comments. Only the change of N, M to R, C is not yet done, see my #8 (comment) on this. But I did change their order and I'm open to arguments, I don't have a super strong opinion on this.

vadixidav
vadixidav approved these changes Apr 30, 2020
View reviewed changes
Copy link
Member

@vadixidav vadixidav left a comment

Choose a reason for hiding this comment

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

Alright, everything has been addressed. I will go ahead and merge! 🎉

@vadixidav vadixidav merged commit db6bbf4 into rust-cv:master Apr 30, 2020
@vadixidav
Copy link
Member

I will release the new version soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadixidav vadixidav vadixidav approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convergence criteria
2 participants
@jannschu @vadixidav