Skip to content

Conversation

thaddeus-pellegrini
Copy link
Contributor

@thaddeus-pellegrini thaddeus-pellegrini commented Apr 25, 2022

Summary

Details and comments

TODO:

Must make tests
Must complete documentation

Comment on lines 58 to 59
if "granularity" in backend.configuration().timing_constraints:
self.granularity = backend.configuration().timing_constraints["granularity"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to do something like this to account for the backend configuration not having timing_constraints:

https://github.com/Qiskit/qiskit-experiments/blob/e06a78bac1c4b8338528e3e6515f5b55c1745f39/qiskit_experiments/library/characterization/cr_hamiltonian.py#L217-L221

wshanks and others added 28 commits July 27, 2022 11:07
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
These should never fire but it doesn't hurt to have them in case
hardware alignment changes in the future
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
@wshanks wshanks marked this pull request as ready for review October 11, 2022 20:21
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @thaddeus-pellegrini @wshanks, overall this looks good to me. Handling of missing backend dt seems bit redundant but apart from this logic I like how this experiment is implemented. I don't really carefully read initial guess algorithm because we can eventually use #896.

Returns:
Circuits for series 0 and 1
"""
circ0, circ1 = self._parameterized_circuits(delay_unit=delay_unit)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we cannot directly generate these circuits inside the method? Personally I don't like having too many (unnecessary) helper functions because it makes reading the code harder. Do you plan to add another experiment with variant of ZZRamsey? Parameterizing pi and dt for handling of missing dt seems overengineering. Alternatively you can call timing helper also inside _parameterized_circuits becasue this method knows self.backend.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The main purpose of splitting _parameterized_circuits from _template_circuits is that I want a method that produces circuits that print nicely. _parameterized_circuits was used to generate the circuit representation in the doc string for example. I tried to make this more clear in a223230 by shifting the parameterizing out into its own method, so it is not part of the .circuits() chain.

@wshanks wshanks requested a review from nkanazawa1989 October 13, 2022 21:31
@wshanks wshanks force-pushed the zz_phase branch 2 times, most recently from 3bde3cc to 63f8ed6 Compare October 14, 2022 00:06
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM


return circ0, circ1

def parametrized_circuits(self) -> Tuple[QuantumCircuit, QuantumCircuit]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The intention of parameterization is much clear now. This looks good to me.

@wshanks wshanks merged commit 0788e1d into qiskit-community:main Oct 18, 2022
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.

3 participants