Skip to content

[very breaking] Fix dual sign of equalities - #235

Open
joaquimg wants to merge 3 commits into
masterfrom
jg/eqdualsign
Open

[very breaking] Fix dual sign of equalities#235
joaquimg wants to merge 3 commits into
masterfrom
jg/eqdualsign

Conversation

@joaquimg

Copy link
Copy Markdown
Member

Close #70 (with some claude help)

@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.18%. Comparing base (565af97) to head (8bbb9ca).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   95.22%   95.18%   -0.04%     
==========================================
  Files          17       17              
  Lines         858      852       -6     
==========================================
- Hits          817      811       -6     
  Misses         41       41              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joaquimg joaquimg changed the title Fix dual sign of equalities [very breaking] Fix dual sign of equalities Aug 24, 2026
Comment thread src/attributes.jl
),
# Classical free variable.
# The value is used as is: dual constraints are built as
# `-A_{.j}^T y + a_j in V_j^*` for every primal variable, whether the

@blegat blegat Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the comment Claude but it won't make sense for a reader of the code without this PR as context ^^
He always does that, it's funny.
Not sure we need to keep it

Comment thread src/attributes.jl

Used as pre-processing for `MOI.set`ting `value` for a variable.
"""
function dual_attribute_value_set end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Glad to get rid of this

```

Note that the equality constraints have minus signs that could be flipped for
simplicity. However, for generality, we keep the minus signs so that the models

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a bit surprised that "we keep the minus sign" is stated before and after the PR. Didn't we change the behavior here ?

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Review sign of dual constraints

2 participants