-
Notifications
You must be signed in to change notification settings - Fork 24
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
Unconstrained Refinement #160
Conversation
This reverts commit a972b3b.
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 77.94% 78.66% +0.72%
==========================================
Files 192 202 +10
Lines 18699 19626 +927
Branches 7589 8015 +426
==========================================
+ Hits 14575 15439 +864
- Misses 4124 4187 +63
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Lgtm. Only some minor changes. If these issues are addressed, feel free to merge (after fixing the windows build) ;-)
config/default_preset.ini
Outdated
# main -> refinement -> fm | ||
r-fm-type=kway_fm | ||
r-fm-type=unconstrained |
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.
I would rename this to "unconstrained_fm"
@@ -57,7 +57,7 @@ i-r-fm-iter-moves-on-recalc=true | |||
# main -> initial_partitioning -> refinement -> flows | |||
i-r-flow-algo=do_nothing | |||
# main -> refinement | |||
r-rebalancer-type=simple_rebalancer | |||
r-rebalancer-type=rebalancer |
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.
did we remove the "simple_rebalancer"? These configs should be not modified as they refer to the state in our dissertations.
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.
I updated the PR so that it doesn't remove the simple rebalancer anymore
@@ -1,123 +0,0 @@ | |||
/******************************************************************************* | |||
* MIT License |
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.
Is there another test for the rebalancer?
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.
added one
@@ -119,6 +119,14 @@ class SparseMapBase { | |||
return _dense[index].value; | |||
} | |||
|
|||
Value operator[] (const Key key) const { |
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 is a confusing semantic of the []-operator. Can we use at(key) function instead?
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.
using getOrDefault(key)
now
// Contains data required for unconstrained FM: We group non-border nodes in buckets based on their | ||
// incident weight to node weight ratio. This allows to give a (pessimistic) estimate of the effective | ||
// gain for moves that violate the balance constraint | ||
struct UnconstrainedFMData { |
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 looks to me more like a class rather than a struct.
const std::vector<HypernodeWeight>& max_part_weights, | ||
vec<vec<Move>>& rebalancing_moves_by_part); | ||
|
||
void insertMovesToBalancePart(const PartitionedHypergraph& phg, |
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.
I prefer the term "block" over "part"
* | ||
*/ | ||
|
||
class GainCacheStrategy { | ||
class LocalGainCacheStrategy { |
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.
move this in a separate file
@@ -72,4 +72,42 @@ class IRefiner { | |||
const double time_limit) = 0; | |||
}; | |||
|
|||
class IRebalancer: public IRefiner { |
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.
move this into "i_rebalancer.h"
|
||
|
||
template <typename TypeTraits, typename GainTypes> | ||
class RebalancerV2 final : public IRebalancer { |
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.
I do not like the naming. Suggestion: "AdvancedRebalancer". Also I would still keep the old rebalancer for our old configurations.
|
||
|
||
template<typename TypeTraits, typename GainTypes> | ||
class UnconstrainedStrategy: public IFMStrategy { |
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.
would split this into two separate files "unconstrained_fm_strategy.h" and "local_unconstrained_fm_strategy.h"
+ simplification in fm strategy
This reverts commit facb8be.
This PR adds the new unconstrained refinement algorithms from our most recent paper (Parallel Unconstrained Local Search for Partitioning Irregular Graphs). Details can be found there.
Summary:
With regards to the code architecture, the new algorithms require the introduction of a new rebalancer interface as well as an interface for the FM strategy. Furthermore, the data for computing approximate penalties is added to the shared FM data.
Results on the medium hypergraph set (k <= 32):
Note that the quality improvement is much more significant on highly irregular graphs (see paper). Specifically, unconstrained has much better quality there than n-level while being almost as fast as the current default configuration. I plan to investigate the results on hypergraphs in more detail within the next week, but I think it makes sense to already open the PR.
Unconstrained has only minimal running time overhead in the multilevel configuration. Consequently, I've added it to the
default
andquality
configurations.In the n-level setting, the overhead is much more significant. This is because unconstrained refinement has some operations (initialization of penalty estimation and rebalancing) which require scanning the whole graph. The consequence is potentially quadratic total work since this can happen for every batch uncontraction. Therefore, the PR does not change the
highest_quality
config. A possible solution is to use unconstrained refinement only for the global refinement rounds. This would probably need some significant changes to the n-level code.Additional notes:
The PR removes the old rebalancer. There seems little point in keeping it, since the new rebalancer outpeforms it