Optim.jl
Optim.jl copied to clipboard
fix: equality comparison where assignment was likely meant
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:
- The equality test would make sense if it was the last statement of the function (in order to return
true
orfalse
), but it's not, which means that the result of comparison is not used. - 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:
-
optimize
first callsupdate_state!
, which seems to leavestate.ev
unchanged (without my "fix"). Then, however, it immediately callsupdate_fg!
: https://github.com/JuliaNLSolvers/Optim.jl/blob/1189ba0347ba567e43d1d4de94588aaf8a9e3ac0/src/multivariate/solvers/constrained/ipnewton/interior.jl#L250-L252 -
update_fg!
, however, does updatestate.ev
: https://github.com/JuliaNLSolvers/Optim.jl/blob/1189ba0347ba567e43d1d4de94588aaf8a9e3ac0/src/multivariate/solvers/constrained/ipnewton/ipnewton.jl#L176-L179 - So equality violation ends up updated anyway...
Codecov Report
Merging #979 (1189ba0) into master (f6850c9) will increase coverage by
0.04%
. The diff coverage is100.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
@@ 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.
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)