Skip to content
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

Pam verify bug fixes #192

Merged
merged 12 commits into from
Oct 17, 2023
Merged

Pam verify bug fixes #192

merged 12 commits into from
Oct 17, 2023

Conversation

edyounis
Copy link
Member

No description provided.

@edyounis edyounis requested a review from jkalloor3 October 15, 2023 22:18
Copy link
Contributor

@jkalloor3 jkalloor3 left a comment

Choose a reason for hiding this comment

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

I think this makes more sense, I just have some questions

@@ -82,10 +82,11 @@ async def run(self, circuit: Circuit, data: PassData) -> None:
if not subgraph.is_fully_connected():
raise RuntimeError('Cannot route circuit on disconnected qudits.')

pi = data.initial_mapping
pi = [i for i in range(circuit.num_qudits)]
Copy link
Contributor

Choose a reason for hiding this comment

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

pi is the permutation of the initial mapping we are solving for? So it gets modified by the forward and backwards passes? Might be helpful to comment this

Copy link
Contributor

Choose a reason for hiding this comment

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

Layout is also called after ApplyPlacement correct? So what good does changing the placement do here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait, ok, in the test_sabre I see that ApplyPlacement is called last, do we need to change this in other workflows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good questions. A few points:

  • The initial and final mappings are always with respect to the original circuit (mapping from original to current), so until the circuit gets modified (ApplyPlacement, Routing), they shouldn't be changed.
  • The current placement tracks the current placement of the current circuit qudits to model qudits, so layout and routing need to start with this information
  • pi always starts as [0, 1, 2, 3, ...], because the placement info is encoded into the data.connectivity call, the actual coupling graph is permuted there to account for it

Yes, ApplyPlacement should be called after SABRE layout and routing. What other workflows are you referring to?

Also, Rather than running GreedyPlacement, another valid placement strategy would be to ApplyPlacement before mapping to "extend" the circuit to the model size, then running layout and hoping SABRE finds a good placement. This is sort of the strategy we take with PAM. We are definitely lacking in the placement department tho

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, so even before we were calling ApplyPlacement before and after PAMLayout and PAMRouting.

else:
pi = [i for i in range(circuit.num_qudits)]

pi = [i for i in range(circuit.num_qudits)]
Copy link
Contributor

Choose a reason for hiding this comment

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

So is the initial mapping always going to 0,1,2,3,... and all of the actual mapping information is in placement? Do we even need initial mapping anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the initial mapping gets updated when we actually apply the placement. Until then, the circuit's input qubit ordering actually doesn't change

@@ -38,12 +39,17 @@ def test_simple(compiler: Compiler) -> None:
GreedyPlacementPass(),
GeneralizedSabreLayoutPass(),
GeneralizedSabreRoutingPass(),
ApplyPlacement(),
]

cc, data = compiler.compile(circuit, workflow, True)
pi = data['initial_mapping']
pf = data['final_mapping']
PI = PermutationMatrix.from_qubit_location(5, pi)
PF = PermutationMatrix.from_qubit_location(5, pf)
Copy link
Contributor

Choose a reason for hiding this comment

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

How did some qudits become inactive? Not sure I understand

Copy link
Member Author

Choose a reason for hiding this comment

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

When you apply the placement, the logical, input circuit size is extended to fit the physical, output machine model. So in this workflow, this means a good portion of them won't be used.

Copy link
Contributor

@jkalloor3 jkalloor3 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -82,10 +82,11 @@ async def run(self, circuit: Circuit, data: PassData) -> None:
if not subgraph.is_fully_connected():
raise RuntimeError('Cannot route circuit on disconnected qudits.')

pi = data.initial_mapping
pi = [i for i in range(circuit.num_qudits)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, so even before we were calling ApplyPlacement before and after PAMLayout and PAMRouting.

@edyounis edyounis merged commit 34114b6 into pam-verify Oct 17, 2023
13 checks passed
@edyounis edyounis deleted the pam-verify-bug-fixes branch October 17, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants