-
Notifications
You must be signed in to change notification settings - Fork 276
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.37% +0.16%
==========================================
Files 349 349
Lines 85465 85957 +492
==========================================
+ Hits 69406 69950 +544
+ Misses 16059 16007 -52 ☔ 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.
|
It would be better to maintain the use of @Opt-Mucca Please make this modification so we can merge this PR |
|
@jajhall Can do. |
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.