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

Implement dynamical decoupling. #6515

Merged
merged 7 commits into from
May 9, 2024
Merged

Implement dynamical decoupling. #6515

merged 7 commits into from
May 9, 2024

Conversation

babacry
Copy link
Collaborator

@babacry babacry commented Mar 20, 2024

Add dynamical decoupling operations to circuits for idle moments.

Two methods are provided to apply dynamical decoupling.

  1. Schema "XX_PAIR" and "YY_PAIR" are provided.
  2. Users can provide their own base dynamical decoupling sequence via base_dd_sequence.

We repeat given dd sequence in continuous idle moments until the remaining idle moments isn't long enough for another repetition.

@babacry babacry requested review from vtomole, cduck and a team as code owners March 20, 2024 21:23
@babacry babacry requested a review from viathor March 20, 2024 21:23
@babacry babacry marked this pull request as draft March 20, 2024 21:23
@babacry
Copy link
Collaborator Author

babacry commented Mar 20, 2024

Post the draft for general suggestions.

And see how the CI goes compared with local CI.

Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

LGTM up to some nits and suggestions. Once this is merged, we can discuss some possible ways to augment it. Thanks!

) -> list['cirq.Gate']:
match schema:
case _DynamicalDecouplingSchema.XX_PAIR:
return _repeat_sequence([cirq.XPowGate(), cirq.XPowGate()], num_idle_moments)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you want cirq.X instead of cirq.XPowGate().

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.

case _DynamicalDecouplingSchema.XX_PAIR:
return _repeat_sequence([cirq.XPowGate(), cirq.XPowGate()], num_idle_moments)
case _DynamicalDecouplingSchema.YY_PAIR:
return _repeat_sequence([cirq.YPowGate(), cirq.YPowGate()], num_idle_moments)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, I think you want cirq.Y here.

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.

raise ValueError('Invalid dynamical decoupling sequence. Expect more than one gates.')
matrices = [cirq.unitary(gate) for gate in dd_sequence]
product = reduce(np.matmul, matrices)
if not np.array_equal(product, np.eye(2)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use cirq.equal_up_to_global_phase() here (since we don't care if the sequence differs from the identity by a global phase).

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.

class _DynamicalDecouplingSchema(enum.Enum):
"""Supported schemes of dynamical decoupling."""

XX_PAIR = 'XX_PAIR'
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may also want one that is does X X**-1. X and X**-1 have the same unitary but are implemented differently on hardware. Similarly for Y.

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.

Copy link

@snichet snichet left a comment

Choose a reason for hiding this comment

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

A couple suggestions for optimal implementation on hardware.

dd_model=DynamicalDecouplingModel.from_schema("XX_PAIR"),
)

# Insert one XX_PAIR dynamical decoupling sequence in idle moments.
Copy link

Choose a reason for hiding this comment

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

should be YY_Pair

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.

return self.schema, self.base_dd_sequence


def add_dynamical_decoupling(
Copy link

Choose a reason for hiding this comment

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

Might be nice to have an option only to insert DD gates on moments that only have single qubits gates. On the hardware, the calibrations for the gates do not include calibrating single qubits and two qubits gates concurrently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, tracking this here. Feel free to put more thoughts there, thanks!

num_idle_moments=moment_id - last_busy_moment_by_qubits[q] - 1
)
for idx, gate in enumerate(insert_gates):
insert_into.append((last_busy_moment_by_qubits[q] + idx + 1, gate.on(q)))
Copy link

Choose a reason for hiding this comment

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

It seems like this is only inserting X gates on consecutive idle moments. I think the next step would be to divide the circuit into moments that are Clifford, then insert X gates on all idle single qubit moments (and perhaps also idle 2 qubit moments, but the caveat is my comment above). Then use the fact that Clifford circuits can be quickly and efficiently simulated to put the correct Clifford gate at the end of the group of moments, to inverse all the previous inserted gates. For CZ gates, this should just be a X, Y or Z gate, i believe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, tracking this here. Feel free to put more thoughts there, thanks!

@babacry babacry marked this pull request as ready for review May 1, 2024 20:30
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.79%. Comparing base (bfba965) to head (b9c9557).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6515      +/-   ##
==========================================
- Coverage   97.79%   97.79%   -0.01%     
==========================================
  Files        1124     1126       +2     
  Lines       95705    95784      +79     
==========================================
+ Hits        93595    93670      +75     
- Misses       2110     2114       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

)


@value.value_equality
Copy link
Collaborator

Choose a reason for hiding this comment

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

it doesn't look like we mutate the properties of this class, consider making this a dataclass/attrs.frozen e.g.

@attrs.frozen
class DynamicalDecouplingModel:
   schema: Optional[_DynamicalDecouplingSchema] = attrs.field(default=None, validator=...)
   base_dd_sequence: Optional[list['cirq.Gate']] = attrs.field(default=None, validator=...) 
   ...etc

actually I don't think this class is needed.. consider removing it, this will simplify your design and make your life easier as you won't need to worry about json

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.

Context: I was not so sure about whether to add a model for dynamical decoupling. I chose to add a model class as I expected the dynamical decoupling schema might get more and more complex in the future. While for now, we only need to handle the case of "Supported schemes" and "Customized sequence". A function is good enough, and we do not need to worry about json.

circuit: 'cirq.AbstractCircuit',
*,
context: Optional['cirq.TransformerContext'] = None,
dd_model: DynamicalDecouplingModel = DynamicalDecouplingModel.from_schema("X_XINV"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of dd_model try

spin_echo_sequence: Sequence[cirq.Gate] = (cirq.X, cirq.X),

Copy link
Collaborator Author

@babacry babacry May 8, 2024

Choose a reason for hiding this comment

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

Updated with schema.

My understanding is "dynamical decoupling sequence" is more general than "spin echo sequence", correct me if I was wrong @eliottrosenberg .

dd_model: DynamicalDecouplingModel = DynamicalDecouplingModel.from_schema("X_XINV"),
) -> 'cirq.Circuit':
"""Add dynamical decoupling gate operations to a given circuit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

does this preserve the moment structure? consider adding this to the docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It preserves the moment structure.

Updated docstring.

@CirqBot CirqBot added the size: L 250< lines changed <1000 label May 5, 2024
Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

just a little bit more

except ValueError:
raise
else:
is_valid, error_message = _validate_dd_sequence(schema)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a golang style, prefer to just raise the error inside _validate_dd_sequence

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.

last_busy_moment_by_qubits: Dict['cirq.Qid', int] = {q: 0 for q in circuit.all_qubits()}
insert_into: list[Tuple[int, 'cirq.OP_TREE']] = []

if isinstance(schema, str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider grouping this logic into a function (e.g. _parse_dd_sequence(schema))

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.

Copy link
Collaborator

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

@babacry congratulations on your first PR

@NoureldinYosri NoureldinYosri merged commit f79f902 into quantumlib:main May 9, 2024
34 checks passed
@babacry
Copy link
Collaborator Author

babacry commented May 10, 2024

Thanks for the careful review, everyone!

jselig-rigetti pushed a commit to jselig-rigetti/Cirq that referenced this pull request May 28, 2024
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
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.

5 participants