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

Translate empty row rule and reduction to apply! interface #13

Merged
merged 8 commits into from
Dec 29, 2020

Conversation

joehuchette
Copy link
Collaborator

No description provided.

src/lp/empty_row.jl Outdated Show resolved Hide resolved
src/lp/empty_row.jl Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Dec 27, 2020

Codecov Report

Merging #13 (f77701b) into master (ab77d9e) will increase coverage by 0.27%.
The diff coverage is 52.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   46.56%   46.83%   +0.27%     
==========================================
  Files          13       14       +1     
  Lines         902      901       -1     
==========================================
+ Hits          420      422       +2     
+ Misses        482      479       -3     
Impacted Files Coverage Δ
src/MathOptPresolve.jl 100.00% <ø> (ø)
src/presolve.jl 45.42% <0.00%> (+0.13%) ⬆️
src/util.jl 0.00% <0.00%> (ø)
src/lp/fixed_variable.jl 80.85% <50.00%> (ø)
src/lp/empty_row.jl 90.00% <62.50%> (-2.16%) ⬇️

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 ab77d9e...f77701b. Read the comment docs.

src/lp/empty_row.jl Outdated Show resolved Hide resolved
Comment on lines 30 to 32
Sets the dual variable $y\_lower_{i} = \max\{0,y\}$ for the greater-than
constraint and the dual variable $y\_upper_{i} = \max\{0,-y\}$ for the
less-than constraint.
Copy link
Owner

Choose a reason for hiding this comment

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

Small clarification.
Unless row i is infeasible, both dual multipliers y_l and y_u are zero: this corresponds to the reduction EmptyRow(i, zero(T)), which is created in the non-infeasible case.

Thus, the y_l = max(0, y), y_u = max(0, -y) in the post-solve only yield non-zero values if the row was infeasible.
My rationale for initially doing so was to have "status-agnostic" reductions and postsolve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the doc, let me know what you think.

Comment on lines 9 to 13
We eliminate row $i$ if, for each variable index $j$,
``math
| a_{i,j} | ≤ ϵ
```
for prescribed tolerance $ϵ$.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see this being done in the code, is it on purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm no, I wrote this first and then never followed through on it. Do we want to do this? If so, at what point? It feels a bit weird to do it in load_problem!: if the user is passing us the matrix with small nonzeros, it seems like we should respect that.

Copy link
Owner

Choose a reason for hiding this comment

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

If we do it, I would rather put it in a different rule, with a drop tolerance for rounding small values to zero and large ones to infinity.
As for the where, once at the beginning of presolve seems reasonable. See also #12 (comment)

joehuchette and others added 3 commits December 27, 2020 20:21
Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
Copy link
Owner

@mtanneau mtanneau left a comment

Choose a reason for hiding this comment

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

Can you add an entry in the docs as well?

src/lp/empty_row.jl Outdated Show resolved Hide resolved
src/lp/empty_row.jl Outdated Show resolved Hide resolved
src/lp/empty_row.jl Outdated Show resolved Hide resolved
joehuchette and others added 2 commits December 28, 2020 19:20
Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
Copy link
Owner

@mtanneau mtanneau left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@joehuchette joehuchette merged commit c317b25 into master Dec 29, 2020
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.

3 participants