Optim.jl icon indicating copy to clipboard operation
Optim.jl copied to clipboard

fix: equality comparison where assignment was likely meant

Open ForceBru opened this issue 2 years ago • 2 comments

As per #976, it looks like in this code in IPNewton:

https://github.com/JuliaNLSolvers/Optim.jl/blob/adc5b277b3f915c25233b45f8f2dd61006815e63/src/multivariate/solvers/constrained/ipnewton/ipnewton.jl#L282-L288

...the line state.ev == equality_violation(constraints, state) probably meant assignment, not an equality test:

  1. The equality test would make sense if it was the last statement of the function (in order to return true or false), but it's not, which means that the result of comparison is not used.
  2. As I understand it, state.ev is supposed to be a number, most likely a floating-point, but comparison of floats for equality can fail, because of rounding errors, so this equality test seems suspicious.

I changed it to assignment, and all tests still pass.


However, it seems like using assignment doesn't change much:

  1. optimize first calls update_state!, which seems to leave state.ev unchanged (without my "fix"). Then, however, it immediately calls update_fg!: https://github.com/JuliaNLSolvers/Optim.jl/blob/1189ba0347ba567e43d1d4de94588aaf8a9e3ac0/src/multivariate/solvers/constrained/ipnewton/interior.jl#L250-L252
  2. update_fg!, however, does update state.ev: https://github.com/JuliaNLSolvers/Optim.jl/blob/1189ba0347ba567e43d1d4de94588aaf8a9e3ac0/src/multivariate/solvers/constrained/ipnewton/ipnewton.jl#L176-L179
  3. So equality violation ends up updated anyway...

ForceBru avatar Feb 08 '22 13:02 ForceBru

Codecov Report

Merging #979 (1189ba0) into master (f6850c9) will increase coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head 1189ba0 differs from pull request most recent head 1dd1dab. Consider uploading reports for the commit 1dd1dab to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #979      +/-   ##
==========================================
+ Coverage   85.35%   85.40%   +0.04%     
==========================================
  Files          42       42              
  Lines        3181     3185       +4     
==========================================
+ Hits         2715     2720       +5     
+ Misses        466      465       -1     
Impacted Files Coverage Δ
src/multivariate/solvers/first_order/bfgs.jl 92.40% <100.00%> (+1.73%) :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 f6850c9...1dd1dab. Read the comment docs.

codecov[bot] avatar Feb 08 '22 13:02 codecov[bot]

I'll have to look at this a bit more in detail as I have to figure out if both are meant to be calculated or only one (thye might for example be the same)

pkofod avatar Feb 10 '22 10:02 pkofod