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

fix: equality comparison where assignment was likely meant #979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ForceBru
Copy link

@ForceBru ForceBru commented Feb 8, 2022

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

# Evaluate the constraints at the new position
constraints.c!(state.constr_c, state.x)
constraints.jacobian!(state.constr_J, state.x)
state.ev == equality_violation(constraints, state)
false
end

...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!:
    update_state!(d, constraints, state, method, options) && break # it returns true if it's forced by something in update! to stop (eg dx_dg == 0.0 in BFGS or linesearch errors)
    update_fg!(d, constraints, state, method)
  2. update_fg!, however, does update state.ev:
    function update_fg!(d, constraints::TwiceDifferentiableConstraints, state, method::IPNewton)
    state.f_x, state.L, state.ev = lagrangian_fg!(state.g, state.bgrad, d, constraints.bounds, state.x, state.constr_c, state.constr_J, state.bstate, state.μ)
    update_gtilde!(d, constraints, state, method)
    end
  3. So equality violation ends up updated anyway...

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

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

❗ 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%) ⬆️

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.

@pkofod
Copy link
Member

pkofod commented Feb 10, 2022

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants