JuliaSmoothOptimizers / BundleAdjustmentModels.jl

Julia repository of bundle adjustment problems
Mozilla Public License 2.0
9 stars 5 forks source link

Corrections issue 39-47-51 #52

Closed AntoninKns closed 2 years ago

AntoninKns commented 2 years ago

Renaming jac_structure! to jac_structure_residual! and jac_coord to jac_coord_residual! to solve #39 and improve compatibility with other modules.

Adding informations in the README.md regarding residual and jac_op_residual functions and the lack of second order information to solve #39.

Changing the docstring to create a BundleAdjustmentModel to solve #39.

Fixing calls to increment! in jac_structure_residual! to solve #47.

Fixing jac_op_residual function by removing call to jac_structure_residual! to solve #51.

codecov[bot] commented 2 years ago

Codecov Report

Merging #52 (1ae8d84) into main (7de12f7) will increase coverage by 0.86%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   90.38%   91.24%   +0.86%     
==========================================
  Files           5        5              
  Lines         312      297      -15     
==========================================
- Hits          282      271      -11     
+ Misses         30       26       -4     
Impacted Files Coverage Δ
src/BundleAdjustmentNLSFunctions.jl 96.19% <100.00%> (+2.85%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7de12f7...1ae8d84. Read the comment docs.

AntoninKns commented 2 years ago

I have added a residual vector in the model here for homogeneity with the jacobian.

I have removed the jac_op_residual function since its content was not corresponding to the one in NLPModels.

I have updated the README.md according to those changes.

The goal of these changes is to be as close as possible to NLPModels in order to adapt to ADNLPModels and other packages that might be connected to this one.

dpo commented 2 years ago

Ok, but I dont think F, rows, cols and vals should be stored inside the model. Other models don't do that.

AntoninKns commented 2 years ago

Ok, so I'll leave them out of the model and in the LevenbergMarquardt package I will put them in the solver.

dpo commented 2 years ago

Yes, exactly.