-
Notifications
You must be signed in to change notification settings - Fork 62
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
Optimize shortestpath in SABRE #1408
Merged
Merged
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
074dcf6
replace deepcopy in looping
687b282
add SWAP undo function
48ed380
add SWAP undo function
6a5b8f4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 79a7bb8
fix error
7539b4a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] b7a84f3
fix bug
9b0f2d0
add test func
a7a1c62
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] fe3144c
add test func
21cb5f7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 7580cf1
separate undo func
18c1f70
update undo remove check
c745fbd
set connectivity of the router in ReverseTraversal
96cb742
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] ceb2ec8
Merge branch 'master' into router_sabre
96a0bd8
Merge branch 'master' into router_sabre
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ok code-wise, the only thing that seems a bit weird to me is that the
undo
requires for you to pass theswap
you wish to undo. I mean, since in principle you undo the last one, you wouldn't need to pass explicitely the swap, but just look for the last one added in a routed block, right? And, indeed, hereswap
is only used to check that the last swap you find is the one you expected.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 used
swap
to double-check if the undo function works correctly. I'll remove the checking step.Also, could you take a look at the question I posted 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.
Sorry for the delay. Double checking could always be useful, indeed, but, if you want to keep it, I wouldn't make it mandatory at least. In the sense that you could have the
swap=None
argument as optional. In any case, this check should be covered in the tests in principle.Regarding the question above, instead, I had a look at it last week. Now, the best person to answer this is probably @Simone-Bordoni, but I believe he is currently on a leave. Thus, I will provide you with my guess, which however may be incorrect. By looking at the
update
functionqibo/src/qibo/transpiler/router.py
Line 241 in a2f7424
it seems that you update the
circuit_logical
mapping, not the physical one (physical_logical
), meaning that in the layout dict, you do not swap the values, but rather the keys.Therefore you start from:
after the
(0, 2)
swap you get:and after
(1, 2)
:This means that you end up with
circuit_logical = [1, 2, 0, 3]
, which is indeed checked in the test:I hope this helped, assuming it is correct. In general, though, this small review of the transpiler code that I did to find the answer, made me think that probably, both, the naming scheme could be improved to make it clearer, and some further documentation should be added to the docstrings. Otherwise, everytime we go back to the code, we risk to waste time in re-understanding everything.
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 for your response. I noticed that while optimizing SABRE and a better naming scheme seems to be needed. I also found and implemented a more efficient way to manage the mapping. I think it would be best to wait until @Simone-Bordoni returns to discuss the new mapping scheme.
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 am currently on leaves. If you want to implement a new mapping scheme you can do the PR and I will review it when I will be back.