-
Notifications
You must be signed in to change notification settings - Fork 743
CTS: improve automatic clustering #8949
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: master
Are you sure you want to change the base?
CTS: improve automatic clustering #8949
Conversation
Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
… max cap Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
…efined Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
Signed-off-by: luis201420 <[email protected]>
…tering Signed-off-by: luis201420 <[email protected]>
|
I ran a CI and all the public designs passed. I am waiting for the results of the private designs. |
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.
clang-tidy made some suggestions
| techChar_(techChar), | ||
| maxInternalDiameter_(10), | ||
| capPerUnit_(0.0), | ||
| use_max_diameter_((HTree->getTreeType() == TreeType::MacroTree) |
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.
warning: no header providing "cts::TreeType" is directly included [misc-include-cleaner]
src/cts/src/SinkClustering.cpp:14:
- #include "stt/SteinerTreeBuilder.h"
+ #include "TreeBuilder.h"
+ #include "stt/SteinerTreeBuilder.h"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 think the code changes look fine, it is good to leave the list of cluster sizes and diameter behind. Just one small suggestion on the method for defining if the limit is exceeded.
| const double capCost, | ||
| const unsigned sizeLimit) | ||
| { | ||
| if (useMaxCapLimit_) { |
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 think this if statement can be a bit different now that we define when to use each of the limits.
suggestion:
bool is_limit_exceeded = false
if (useMaxCapLimit_) {
is_limit_exceeded |= (capCost > options_->getSinkBufferInputCap() * max_cap__factor_);
// size is defined by the user
if (use_max_size_) {
is_limit_exceeded |= (size >= sizeLimit);
}
// diameter is defined by the user
if (use_max_diameter_) {
is_limit_exceeded |= (cost > maxInternalDiameter_);
}
return is_limit_exceeded;
like this we don't need to separate into 2 different cases useMaxCap and not useMaxCap
| void setMaxDiameter(double distance) | ||
| { | ||
| maxDiameter_ = distance; | ||
| sinkClusteringUseMaxCap_ = false; | ||
| maxDiameterSet_ = true; | ||
| } |
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.
By the comment of the PR we would only want to disable buffer max cap if both cluster size and diameter were set. But here if we set the diameter we immediately disable the useMaxCap_.
| void setSinkClusteringSize(unsigned size) | ||
| { | ||
| sinkClustersSize_ = size; | ||
| sinkClusteringUseMaxCap_ = false; | ||
| sinkClustersSizeSet_ = true; | ||
| } |
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.
Same here.
Fixes #8540
It eliminates the need for predefined lists to find the best combination of diameter and cluster size. Both lists are no longer needed since buffer max cap includes them. The iterations that performed this search are also removed.
The following cases will now be considered as follows: