-
Notifications
You must be signed in to change notification settings - Fork 269
Order path separator #2674
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
base: latest
Are you sure you want to change the base?
Order path separator #2674
Conversation
This reverts commit 6f62e0e.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## latest #2674 +/- ##
==========================================
+ Coverage 81.20% 81.25% +0.04%
==========================================
Files 349 349
Lines 85465 85801 +336
==========================================
+ Hits 69406 69721 +315
- Misses 16059 16080 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fwesselm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @Opt-Mucca. I will try to qualify as soon as possible.
| i != lp.a_matrix_.start_[col + 1]; ++i) { | ||
| HighsInt row = lp.a_matrix_.index_[i]; | ||
| if (rowtype[row] == RowType::kUnusuable) continue; | ||
| double val = lp.a_matrix_.value_[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have
double absval = std::abs(lp.a_matrix_.value_[i]);
here and use this in the following two lines (instead of having the std::abs twice)?
| std::make_pair(-1, HighsImplications::VarBound())); | ||
| boundTypes.resize(numTransformedCol); | ||
| vectorsum.setDimension(numTransformedCol); | ||
| colFractionality.resize(lprelaxation.numCols()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This vector does not have to be re-initialized (with zeros), correct?
|
|
||
| if (lbDist[col] <= ubDist[col] && lbDist[col] == 0 && | ||
| bestVlb[col].first != -1) { | ||
| const double frac = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an utility
inline T fractionality(T input, T* intval = nullptr)
in HighsLpUtils.h that you could maybe use here.
This PR sorts rows via their
fractionalActivityinHighsPathSeparatorwhen considering which row to add to the aggregation.High level overview: The separator tries to find a continuous column to aggregate out, and to do so finds an additional row that also contains that continuous column. Which row to use is non-obvious.
The old method:
The new method:
Pros:
array[i-1]is checked beforearray[i]in all cases aside from starting the scan ati.Cons:
This should only be merged after doing a performance run.