IBM-Quantum-Technical-Enablement / quantum-enablement

IBM Quantum Engineering and Enabling Technologies
Apache License 2.0
26 stars 8 forks source link

docs(notebooks): add pulse-efficient transpilation guide #5

Closed haimeng-zhang closed 3 months ago

haimeng-zhang commented 5 months ago

Summary

Add the how-to guide on pulse-efficient transpilation for review. Working on adding tests per contribution guideline.

Details and comments

pedrorrivero commented 5 months ago

Thanks @haimeng-zhang!

Would you mind creating a notebooks directory inside of docs and placing yours inside? (i.e. ./docs/notebooks/<NOTEBOOK>)

pedrorrivero commented 4 months ago

Hi @haimeng-zhang can you check that this notebook runs with a freshly created environment and only this repo installed from source? Please make sure to install the notebook optional dependencies.

Since Friday, this should be equivalent to Qiskit 1.0.0. You will need to rebase this branch to main, let me know if you need help.

If you need additional dependencies please flag.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 8607305174

Details


Totals Coverage Status
Change from base Build 8573345067: 0.0%
Covered Lines: 290
Relevant Lines: 290

💛 - Coveralls
nathanearnestnoble commented 4 months ago

Hello!

haimeng-zhang commented 3 months ago

This is a great notebook to be published. But a few points to update this with the latest requirements:

  1. The notebook doesn't follow Qiskit Pattern. Currently the four steps prescribed are - Map problem to quantum native format, Optimize circuits, Execute using a quantum primitive, Post-process and analyze results (look into Mirko's Krylov notebook for reference). The four steps in this notebook in this pattern can be generating the trotterized circuit (step 1), transpilation using Rzx (step 2), execute (step 3), show comparison (step 4).
  2. The notebook uses older version of Qiskit and Qiskit Runtime. It might be good to update it to the latest version (although I doubt any significant change in code is required). The final execution should use EstimatorV2.
  3. We are not encouraged to use the "transpile" function anymore. All the "transpile" should be replaced by "generate_preset_pass_manager" or any other pass manager as required.
  4. Remove the comments on # TODO (cell 11 and 16).
  5. Cell 11 raises an important concern that all current hardware use ECR. It might be useful to show results with ECR gates, otherwise this becomes restricted to only Falcon devices.
  6. Since RZX may not be the most familiar gate (it is not familiar to me at least), is it possible to describe the operation of this gate a bit more and add reference?
  7. The circuit drawn after cell 6 has a single RZX gate, but the notes below states 3 RZX gates.

Thanks for the review! Here is a summary of the updates according to the comments above:

  1. Subtitles updated to follow Qiskit Pattern
  2. Jobs with pulse gates failed when updated to EstimatorV2 with the following error ERROR Failed to execute program: Instruction rzx is not supported - hence we choose to use EstimatorV1 in the current implementation and will update to EstimatorV2 when this issue is resolved in next release of Qiskit Runtime.
  3. TODO comments removed
  4. Updated transpilation method to support ECR gates
  5. Added references in the first paragraph for RZX gate
  6. Updated the notes on the number of RZX gates after transpilation

Regarding comment 3, I am unsure about the motivation behind using the "generate_preset_pass_manager" instead of the "transpile" function. I will follow up on this comment after more investigation.

miamico commented 3 months ago

I have similar comments as Ritajit's. Throughout the notebook there is a swinging back and forth from CX to ECR. Let's pick one and stick with it. Other than that, looking good!

haimeng-zhang commented 3 months ago

Hi Haimeng, thanks for addressing most of the reviews. I wanted to draw your attention to a few minor things:

  1. The notes just before Qiskit Pattern Step 1 still talks about CNOT gates, please update the notes to mention ECR instead of CNOT.
  2. We are suggested to use generate_preset_pass_manager instead of transpile. So please change all the transpile commands to this.
  3. After the two transpilations, one using ECR and the other using RZX, the note again mentions CNOT. Please update with ECR in the notes (currently cell 73). CNOT is again mentioned in the notes after cell 74.
  4. In the cell on "estimate circuit duration" - (i) replace transpile with generate_preset_pass_manager, and replace comments to include ECR instead of CNOT/CX.
  5. Replace CNOT with ECR in the notes right after this cell. There are a few more cells where the notes mention CNOT/CX.
  6. In the plot, please change the label from cx to ecr.
  7. In the larger example also, change the transpile to generate_preset_pass_manager.

Hi Haimeng, thanks for addressing most of the reviews. I wanted to draw your attention to a few minor things:

  1. The notes just before Qiskit Pattern Step 1 still talks about CNOT gates, please update the notes to mention ECR instead of CNOT.
  2. We are suggested to use generate_preset_pass_manager instead of transpile. So please change all the transpile commands to this.
  3. After the two transpilations, one using ECR and the other using RZX, the note again mentions CNOT. Please update with ECR in the notes (currently cell 73). CNOT is again mentioned in the notes after cell 74.
  4. In the cell on "estimate circuit duration" - (i) replace transpile with generate_preset_pass_manager, and replace comments to include ECR instead of CNOT/CX.
  5. Replace CNOT with ECR in the notes right after this cell. There are a few more cells where the notes mention CNOT/CX.
  6. In the plot, please change the label from cx to ecr.
  7. In the larger example also, change the transpile to generate_preset_pass_manager.

Thank you for the review! I have

  1. replaced references to the CXGate with the ECRGate
  2. removed the usage of the qiskit.transpile() function and used the preset pass managers instead - this way we provide more information on the subroutines inside of the transpilation step.

All comments above are addressed now.

haimeng-zhang commented 3 months ago

@haimeng-zhang thanks for making the changes. Two minor observations:

  1. The note after cell 262, did you mean InstructionScheduleMap to be a link? If yes, then please insert the link. If not, then the bracket is not required.
  2. For your smaller example, you used optimization_level=3 for transpilation. But when you moved to bigger example with 21 qubits, you used optimization_level=2 for transpilation. Is there a reason for this? If yes, would you give a comment on this? If not, it's better to be consistent.

Appreciate the review!

  1. The missing url is inserted;
  2. All transpilation now uses optimization_level=2 to be consistent.

All comments above have been addressed.