Skip to content

Implements the LineInititialMapper strategy#5831

Merged
CirqBot merged 72 commits intoquantumlib:masterfrom
ammareltigani:routing-line_initial_mapper
Aug 27, 2022
Merged

Implements the LineInititialMapper strategy#5831
CirqBot merged 72 commits intoquantumlib:masterfrom
ammareltigani:routing-line_initial_mapper

Conversation

@ammareltigani
Copy link
Copy Markdown
Contributor

This PR implements the main initial mapping strategy highlighted in http://tinyurl.com/cirq-qubit-routing

…q' namespace; made both classes not serializable
Comment thread cirq-core/cirq/transformers/__init__.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated

@value.value_equality
class LineInitialMapper(routing.AbstractInitialMapper):
"""Places logical qubits in the circuit onto physical qubits on the device."""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a brief description to class docstring as well, highlighting high level details like an overview of strategy, expected complexity etc.

Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
@ammareltigani
Copy link
Copy Markdown
Contributor Author

@tanujkhattar A few notes:

  1. The indeterminism issue stemming from different edge ordering can be resolved by sorting all the edges of the incoming device graph in the constructor in the same way as 'MappingManager'. Given this it might make sense to make the class a value_equality since we have the sorted graph anyway. This obviously introduces some overhead in the __init__ method so another solution would be to sort the nodes given by each iteration of bfs_successors() in the _find_closest_unmapped_qubit() helper. Not sure what is more costly in this tradeoff.

  2. There isn't an exact guarantee that is placement strategy will always make the 2-qubit gates in the first two timesteps executable. The exceptions are 2-qubit gates with qubits incident to an edge that was removed in cycle breaking. Here is a quote from https://arxiv.org/pdf/1902.08091.pdf : . This
    ensures that most of the gates in the first two timesteps can be applied without any swaps; the
    only exceptions are those gates corresponding to the edges removed when breaking rings.

@tanujkhattar
Copy link
Copy Markdown
Collaborator

  1. The indeterminism issue stemming from different edge ordering can be resolved by sorting all the edges of the incoming device graph in the constructor in the same way as 'MappingManager'. Given this it might make sense to make the class a value_equality since we have the sorted graph anyway. This obviously introduces some overhead in the __init__ method so another solution would be to sort the nodes given by each iteration of bfs_successors() in the _find_closest_unmapped_qubit() helper. Not sure what is more costly in this tradeoff.

IIUC, the indeterminism is in graph construction in the tests then? Anyways, sorting once in the constructor to get rid of sorting in each bfs_successors and get_next_physical_qubit sounds good to me.

Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
# Greedily map to highest degree neighbor that that is available
sorted_neighbors = sorted(
self.device_graph.neighbors(current_physical),
key=lambda x: self.device_graph.degree(x),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be sorting based on "unused degree", i.e. x > y if x has more unmapped neighbors than y?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep note of this change and try it during benchmarking!

return neighbor
# If cannot map onto one long line of physical qubits, then break down into multiple
# small lines by finding nearest available qubit to the physical center
return self._closest_unmapped_qubit(self.center)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we find a closest unmapped qubit to current_physical instead? If the outer loop of logical qubits is iterating over a connected line, then this would ensure that we find the closest node to the previous neighbour in line (i.e. current_physical), instead of forgetting about the line position and restarting from center?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep note of this change and try it during benchmarking!

Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
ammareltigani added a commit to ammareltigani/Cirq that referenced this pull request Aug 25, 2022
Copy link
Copy Markdown
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nits

Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper_test.py Outdated
Comment thread cirq-core/cirq/transformers/routing/line_initial_mapper.py Outdated
@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 27, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 27, 2022
@CirqBot CirqBot merged commit 950d2d4 into quantumlib:master Aug 27, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 27, 2022
@ammareltigani ammareltigani deleted the routing-line_initial_mapper branch August 28, 2022 18:54
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
This PR implements the main initial mapping strategy highlighted in http://tinyurl.com/cirq-qubit-routing
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
This PR implements the main initial mapping strategy highlighted in http://tinyurl.com/cirq-qubit-routing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: L 250< lines changed <1000

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants