-
Notifications
You must be signed in to change notification settings - Fork 407
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
[APPack] Flat-Placement Informed Unrelated Clustering #2947
[APPack] Flat-Placement Informed Unrelated Clustering #2947
Conversation
ec44675
to
14f7914
Compare
} | ||
} | ||
|
||
// Push the neighbors of the position to the queue. |
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.
Why not include the diagonal neighbors of the tile as well?
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.
We are doing the manhattan / L1 distance here, not the euclidean / L2 distance; technically the diagonals are 2 away from the current tile, not one. Even in euclidean space, the diagonals are 1.4 distance away, so we want to check the direct neighbors before attempting the diagonals anyways.
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 see your point here. I was just thinking that, considering how tiles are placed on the FPGA, it might not make much difference whether a molecule is taken from a tile directly above or from a tile in the top-right corner.
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.
That is true; however throughout the AP flow I use the L1 norm. In theory it should mean that diagonals are worst than up and down! Perhaps its something to investigate in the future!
@@ -0,0 +1,54 @@ | |||
############################################################################### |
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.
Do we have a test when IO blocks are not fixed?
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 just the case I want to support the most right now. Its also simple to add since I already have the fixed IOs for these testcases. I still consider not providing IO blocks an experimental feature.
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.
Got it. When we were discussing the changes, you mentioned that you might want to claim the feature that IOs don’t need to be fixed. I just brought this up to check whether this is a feature you’d like AP to support or not. If it is supported, we should have a corresponding test for it.
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.
Used flat placement information provided by APPack to try and select better unrelated candidates. This searches for candidates as close to the flat placement position of the cluster. There are two parameters that control how this is performed: 1) max_unrelated_tile_distance decides how far the algorithm will search for unrelated candidates. The algorithm will check for candidates in the same tile as the cluster, and then will search farther and farther out 2) max_unrelated_clustering_attempts decides how many failing attempts the cluster will try unrelated clustering. This matches the option of the same name in the candidate selector class; but this was made separate since likely it will be different for APPack.
14f7914
to
ef52ece
Compare
@amin1377 Do you have any further comments on this PR (or any push-back on my explanations above)? |
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.
Thanks, Alex. The PR looks good to me. Feel free to merge it if you've made all the changes you wanted.
Used flat placement information provided by APPack to try and select better unrelated candidates. This searches for candidates as close to the flat placement position of the cluster.
There are two parameters that control how this is performed:
max_unrelated_tile_distance decides how far the algorithm will search
for unrelated candidates. The algorithm will check for candidates in
the same tile as the cluster, and then will search farther and
farther out
max_unrelated_clustering_attempts decides how many failing attempts
the cluster will try unrelated clustering. This matches the option of
the same name in the candidate selector class; but this was made
separate since likely it will be different for APPack.