Skip to content

Conversation

@mpharrigan
Copy link
Collaborator

@mpharrigan mpharrigan commented Apr 4, 2022

Add a QubitPlacer that takes an explicit mapping in its constructor.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Apr 4, 2022
@mpharrigan mpharrigan force-pushed the 2022-01-qubit-placement-c branch from cf5e14f to 051733c Compare April 4, 2022 22:31
@mpharrigan mpharrigan force-pushed the 2022-01-qubit-placement-c branch from 051733c to 284cbb7 Compare April 18, 2022 21:36
@mpharrigan mpharrigan requested a review from tanujkhattar April 18, 2022 21:36
@mpharrigan mpharrigan marked this pull request as ready for review April 18, 2022 21:36
@mpharrigan mpharrigan requested review from a team, cduck, verult, vtomole and wcourtney as code owners April 18, 2022 21:36
@tanujkhattar tanujkhattar self-assigned this Apr 18, 2022
Copy link
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.

Looking good overall. Left a first round of comments.

max_plots: int = 20,
axes: Sequence[plt.Axes] = None,
tilted=True,
bad_placement_callback=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Add type annotation (preferably also to the tilted argument above)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done; thanks



class HardcodedQubitPlacer(QubitPlacer):
def __init__(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Add docstrings to the class, init and other public methods like place_circuit (if the inherited docstring is not enough).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, sorry

@mpharrigan
Copy link
Collaborator Author

@tanujkhattar ptal

Copy link
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

@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 21, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Apr 21, 2022

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 21, 2022
@mpharrigan mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 22, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 22, 2022
@CirqBot CirqBot merged commit 27abfdb into quantumlib:master Apr 22, 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 Apr 22, 2022
tonybruguier pushed a commit to tonybruguier/Cirq that referenced this pull request Apr 22, 2022
Add a `QubitPlacer` that takes an explicit mapping in its constructor.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Add a `QubitPlacer` that takes an explicit mapping in its constructor.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Add a `QubitPlacer` that takes an explicit mapping in its constructor.
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.

3 participants