Skip to content

Conversation

@alexanderivrii
Copy link
Member

Summary

With several open PRs on improving the performance of the LitinskiTransformation pass, it might be a good idea to add it to ASV benchmarking. The test is based on the script by @mtreinish from this comment.

Details and comments

For reference, here is the command that I am typically running to compare my local

branch and my local <development_branch>:
asv continuous -E virtualenv-py3.13 --show-stderr --interleave-processes --split --no-only-change main <development_branch> --bench LitinskiTransformationPassBenchmarks

@alexanderivrii alexanderivrii requested a review from a team as a code owner November 27, 2025 14:26
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@ShellyGarion ShellyGarion added the Changelog: None Do not include in changelog label Nov 27, 2025
@ShellyGarion ShellyGarion added this to the 2.3.0 milestone Nov 27, 2025
@ShellyGarion
Copy link
Member

This PR should be useful for #15315 and #15318

@coveralls
Copy link

coveralls commented Nov 27, 2025

Pull Request Test Coverage Report for Build 19797647745

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 150 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.009%) to 88.382%

Files with Coverage Reduction New Missed Lines %
crates/cext/src/transpiler/passes/remove_identity_equiv.rs 1 79.17%
crates/circuit/src/parameter/parameter_expression.rs 1 82.3%
crates/circuit/src/parameter/symbol_expr.rs 1 73.39%
qiskit/circuit/commutation_checker.py 1 94.74%
crates/transpiler/src/passes/commutation_analysis.rs 4 92.78%
crates/qasm2/src/lex.rs 7 91.52%
crates/qasm2/src/parse.rs 12 96.62%
crates/transpiler/src/passes/remove_identity_equiv.rs 12 90.7%
qiskit/circuit/library/pauli_evolution.py 21 81.94%
crates/transpiler/src/commutation_checker.rs 22 88.84%
Totals Coverage Status
Change from base Build 19723813215: 0.009%
Covered Lines: 95671
Relevant Lines: 108247

💛 - Coveralls

@Cryoris
Copy link
Collaborator

Cryoris commented Nov 28, 2025

It would be good to cover structurally different circuits here. For starters we could add a Hamiltonian simulation circuit (e.g. Heisenberg on a line and square lattice) and a Grover search circuit, plus extend to some other interesting building blocks later. I wouldn't block #15318 on this though, given that RC1 is next week.

Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

I agree that we can add further circuits to the benchmark but we can also do this in a follow-up PR. How much time does it take compared to other benchmarks? 1000 qubits are not too much?

@alexanderivrii
Copy link
Member Author

Thanks @Cryoris, agreed I would not block other PRs on this. Anyhow, I have realized that we have a variety of QASM files including a square Heisenberg example, and I have included them in this PR.

Creating a Grover example on many qubits seems tricky, as the default flow for the oracle x_1 & ... & x_100 from your demo gets stuck building the truth table.

Shelly, I will post current ASV results for #15318 in that thread, but short answer: it seems that 1000 qubits is fine, especially after that PR.

Ideally, I would like to combine the two classes that I am currently adding into one, but I didn't figure out if I can have the same ASV class to include both parametric and non-parametric benchmarks but avoid to call non-parametric setup / run non-parametric benchmarks multiple times.

Copy link
Member

@ShellyGarion ShellyGarion 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 is a good benchmark. Let's see if @Cryoris has any further suggestions.

@ShellyGarion ShellyGarion added the type: qa Issues and PRs that relate to testing and code quality label Nov 30, 2025
self.qasm_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "qasm")
self.basis_gates = ["rz"] + get_clifford_gate_names()

self.qft = self.build_from_qasm("qft_N100.qasm")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already test QFT above 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

We may want to change the "above" QFT to include Z-measurements. Also, the "above" QFT measures the effect on scaling, and this QFT is a standalone (but I am happy to remove it, if you wish).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I would prefer to remove it, there's not really any additional information here 🤔

Comment on lines +236 to +237
self.dtc = self.build_from_qasm("dtc_100_cx_12345.qasm")
self.eoh = self.build_from_qasm("test_eoh_qasm.qasm")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these two circuits?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I don't know, I was simply looking for adding more benchmarks, and we use already these benchmarks for transpiler and transpiler-levels.

We have this comment somewhere Generate a Floquet unitary for DTC evolution, so dtc is some kind of Hamiltonian evolution benchmark for discrete time crystals. I don't know anything about eoh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably know what circuits we're benchmarking here to make sure it's cases we're interested in, since those are the metrics we want to optimize.

Copy link
Member

@ShellyGarion ShellyGarion Dec 1, 2025

Choose a reason for hiding this comment

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

eoh is a 2-qubit very deep circuit, which perhaps won't be too relevant for Clifford+T pipeline. perhaps it could be removed from here?

Copy link
Member

Choose a reason for hiding this comment

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

Specifically it is a naively (I say this because you could run two qubit peephole on the circuit to reduce it to 7 gates) transpiled two qubit evolution of hamiltonian (eoh) circuit from the early days (so it's all in terms of u1, u2, u3, and cx). It has ~3500 gates in the circuit and it was a scale test in the early days because it was too large for qiskit in the very early days and transpiling it again took a very long time.

CollectMultiQBlocks(max_block_size).run(self.dag)


class LitinskiTransformationPassQFTBenchmarks:
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 maybe dedicate this class to scaling or so -- it's more generic that "QFT" 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

A few questions:

So you are fine with the idea of having two classes - one for parametric benchmarks (not QFT-only) and one for specific circuits (not Qasm-only)?

Do we want to add Z-measurements to some of the circuits (currently none of the selected Qasm files have these)?

Do we want to benchmark using both values for fix_cliffords?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do scaling plots with ASV? If yes, two classes might be nice to generate that separate data. If no, then I think we could just have a single class.

Re: measurements - yes we should add measurements too, to test the PPM Rust path.

Re: fix_cliffords - I think we should check both, yeah

@Cryoris Cryoris modified the milestones: 2.3.0, 2.4.0 Dec 10, 2025
@Cryoris Cryoris removed this from Qiskit 2.3 Dec 10, 2025
@Cryoris
Copy link
Collaborator

Cryoris commented Dec 10, 2025

With these open questions on what to include there's not really time left to merge for 2.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in changelog type: qa Issues and PRs that relate to testing and code quality

Projects

Status: Ready

Development

Successfully merging this pull request may close these issues.

6 participants