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.
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
HLS with coupling map #9250
HLS with coupling map #9250
Changes from 6 commits
6f82985
646ca0d
57b1c7b
38a66a5
7c05f15
2c6692c
243027c
a560566
ea2ff5e
482450e
07e4881
8363767
038d335
33c9764
33f7e5d
e044be1
a87d495
f5631a9
6a1ab8d
af6658f
cf8b9e1
1266e1f
a324253
37aa551
13cbc6e
0e9b95a
c5599df
4e1b1e0
9738a21
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For backwards compatibility I would reverse these arguments. Python will let you specify keyword arguments positionally by default so putting
coupling_map
first will break users doingHighLevelSynthesis(HLSConfig(...))
which was valid way to instantiate the pass before.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.
Can we also add a
Target
argument here, similar to what I do in #9263 ? Basically I'm trying to unify our internal transpiler model around the target. My plan for 0.24.0 is to hopefully have the preset pass manager only use a target internally.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.
@mtreinish, so should right now
HighLevelSynthesis
accept bothcoupling_map
andtarget
, or justtarget
? I guess I am asking whethergenerate_translation_passmanager
(or code further upstream) updatestarget
withcoupling_map
information if only the latter is specified.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.
Right now it needs to accept both, there is still a code path in
transpile
where a target will not be generated (I was hoping to change this for 0.24, so it was only target, but it won't make it). You can do it with a second argument or what we did in #9263 is had a single argument take either a coupling map or a target.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.
It might be more efficient to use rustworkx's
dfs_search()
method to do this traversal: https://qiskit.org/documentation/retworkx/apiref/rustworkx.dfs_search.html#rustworkx.dfs_search which lets you build this as an event driven iterator on the rust side. So you define a visitor class which has hook points in the dfs and rustworkx calls the visitor methods based on it's dfs traversal.But we definitely should do this in rustworkx natively for the next release because this algorithm can be implemented in parallel fairly easily.
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, I did not know about the rustworkx's
dfs_search
method. Though, here we need something akin to "DFS with backtracking", and I was not quite able to figure out ifdfs_search
can be used for that; that is, can this be implemented with some appropriate visitor? In any case, this function is only temporary as I am planning to follow your suggestion of adding it to rustworkx.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.
It might be something that can implement with a specially crafted visitor, but I'm not sure. It's at least not obvious to me quickly scanning the docs and the code. Maybe @georgios-ts knows (as he wrote the functions), but yeah as a temporary step this is fine while waiting on a dedicated function in rustworkx.
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.
dfs_search
cannot be used since it does not support backtracking as @alexanderivrii correctly pointed out. Butall_pairs_all_simple_paths
might be a viable option: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, @georgios-ts! I have already noticed the min-depth option, see my comment here: #9250 (comment). What scares me is what if there is an exponential number of hamiltonian paths, is there a way to limit rustworkx to compute only some fixed number of them?