Oxidize `NLocal` & family #13310

Cryoris · 2024-10-11T06:41:50Z

Summary

Part of #13046: create the n_local function and port the internals to Rust. This will include other parts of the NLocal family, such as TwoLocal, EfficientSU2, RealAmplitudes etc. EvolvedOperatorAnsatz is potentially a bit more involved and might be done in a follow-up.

Details and comments

The new code is particularly fast when dealing with gates we already have in Rust and that don't need any special re-parameterization. In this case:

the new code is ~22x faster for 100 qubits, full entanglement and 10 reps

num qubits: 10, speedup: 9.075
main: 0.005 +- 0.004
this: 0.001 +- 0.000

num qubits: 50, speedup: 18.138
main: 0.063 +- 0.014
this: 0.003 +- 0.003

num qubits: 100, speedup: 22.011
main: 0.251 +- 0.034
this: 0.011 +- 0.007

num qubits: 200, speedup: 26.829
main: 0.957 +- 0.034
this: 0.036 +- 0.010

But if custom gates or special parameterizations are used (e.g. RXGate(x + 3 * y)) we need to go back to Python space to correctly handle the parameter logic. This slows down the code and in this case:

the new code is ~3.5x faster for 100 qubits, linear entanglement (full took too long) and 10 reps

num qubits: 10, speedup: 4.594
main: 0.027 +- 0.006
this: 0.006 +- 0.000

num qubits: 50, speedup: 3.947
main: 0.162 +- 0.006
this: 0.041 +- 0.011

num qubits: 100, speedup: 3.666
main: 0.366 +- 0.031
this: 0.100 +- 0.019

To do

Improve docstrings
Make the entanglement handling rust-side a bit better to understand instead of a 4-times nested vec
~~Use n_local in NLocal (i.e. fast path for Python code)~~ This might not be possible since NLocal works with QuantumCircuits as blocks, but n_local needs gates. Functionally it'd be the same but the circuits are nested differently, which is not backward compatible. We could add a fast-path in case the inputs are not circuits but gates.
Add the family members, like efficient_su2 etc.

coveralls · 2024-10-11T07:08:08Z

Pull Request Test Coverage Report for Build 11723083102

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

530 of 530 (100.0%) changed or added relevant lines in 14 files are covered.
843 unchanged lines in 32 files lost coverage.
Overall coverage increased (+0.1%) to 88.87%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/target.py	1	96.13%
qiskit/circuit/parameter.py	1	98.36%
qiskit/synthesis/unitary/qsd.py	1	97.58%
qiskit/compiler/transpiler.py	1	93.14%
qiskit/circuit/library/arithmetic/adders/draper_qft_adder.py	1	96.97%
qiskit/circuit/library/generalized_gates/gms.py	2	94.44%
qiskit/circuit/library/generalized_gates/rv.py	2	84.62%
qiskit/transpiler/passes/optimization/consolidate_blocks.py	2	96.15%
crates/qasm2/src/lex.rs	3	92.48%
qiskit/transpiler/preset_passmanagers/generate_preset_pass_manager.py	3	97.83%

Totals
Change from base Build 11683622570:	0.1%
Covered Lines:	78307
Relevant Lines:	88114

💛 - Coveralls

missing Mr EvolvedOperatorAnsatz and Ms QAOAAnsatz

qiskit-bot · 2024-10-17T13:33:59Z

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

@Cryoris
@Qiskit/terra-core
@ajavadia

crates/accelerate/src/circuit_library/mod.rs

crates/accelerate/src/circuit_library/n_local/blocks.rs

mtreinish · 2024-10-30T15:03:50Z

crates/accelerate/src/circuit_library/n_local/blocks.rs

+/// their underlying representation (e.g. a string or a list of connections) and returning
+/// these when given a layer-index.
+pub struct Entanglement {
+    entanglement_vec: Vec<LayerEntanglement>,


So it's Vec<Vec<Vec<u32>>?

Correct, we can think of the structure as

Connection: Vec<u32> // indices that the multi-qubit gate acts on BlockEntanglement: Vec<Connection> // entanglement for single block LayerEntanglement: Vec<BlockEntanglement> // entanglements for all blocks in the layer Entanglement: Vec<LayerEntanglement> // entanglement for every layer

Maybe this would be nice to add as explanatory comment?

A comment wouldn't hurt. I assume you're using the type aliases here to make it simpler. It would be easier for me personally to understand of Vec<u32> Vec<Vec<u32>>, etc rather than Connection, BlockEntanglement, etc. But I guess cargo doesn't like that. Either way if you want to add a comment that would be nice for people reading the code.

yeah I also found myself going back checking whether BlockEntanglement is now a 2 or 3 times nested Vec, but I think in the end the explanation plus the types might make it a bit easier to read 🙂

crates/accelerate/src/circuit_library/n_local/multi_local.rs

mtreinish · 2024-10-30T16:41:39Z

crates/accelerate/src/circuit_library/n_local/multi_local.rs

+        if !insert_barriers {
+            Ok(Self { barrier: None })
+        } else {
+            let barrier_cls = imports::BARRIER.get_bound(py);


I'm looking forward to having a solution for #12966 so we can do this without needing python.

qiskit/circuit/library/n_local/efficient_su2.py

mtreinish · 2024-10-30T16:49:55Z

qiskit/circuit/library/n_local/excitation_preserving.py

        >>> ansatz = ExcitationPreserving(3, reps=1, insert_barriers=True, entanglement='linear')
-        >>> print(ansatz)  # show the circuit
+        >>> print(ansatz.decompose())  # show the circuit


Did you want to use flatten here instead so you don't need to add the decompose() call

I'll add an example with flatten, but would also like to show the decompose flow 😄

Why though? My concern here is that doing decompose() here is the less efficient path so we're implicitly encouraging a pattern that will result in subpar performance. But we have the seealso now pointing people to the faster function so that's probably enough. It's not worth blocking over either way, so I'm fine if you don't want to change this.

I thought just one example of decompose would be good so users know how to handle the "vanilla" format of the circuits, where one just gives the minimal amount of argument possible 😄 Except the first example, all others now use flatten 🙂

qiskit/circuit/library/n_local/n_local.py

ShellyGarion

LGTM. I have a few questions ?

crates/accelerate/src/circuit_library/multi_local.rs

qiskit/circuit/library/__init__.py

qiskit/circuit/library/n_local/efficient_su2.py

qiskit/circuit/library/n_local/excitation_preserving.py

qiskit/circuit/library/n_local/n_local.py

qiskit/circuit/library/n_local/real_amplitudes.py

mtreinish · 2024-11-06T19:33:30Z

test/python/circuit/library/test_nlocal.py

@@ -49,6 +54,18 @@
 from test import QiskitTestCase  # pylint: disable=wrong-import-order


+class Gato(Gate):


Si, correcto 😛

mtreinish · 2024-11-06T19:38:53Z

qiskit/circuit/library/n_local/excitation_preserving.py

        >>> ansatz = ExcitationPreserving(3, reps=1, insert_barriers=True, entanglement='linear')
-        >>> print(ansatz)  # show the circuit
+        >>> print(ansatz.decompose())  # show the circuit


Why though? My concern here is that doing decompose() here is the less efficient path so we're implicitly encouraging a pattern that will result in subpar performance. But we have the seealso now pointing people to the faster function so that's probably enough. It's not worth blocking over either way, so I'm fine if you don't want to change this.

mtreinish

I think I'm mostly happy with this. There is a lot of code so I'm not 100% convinced I kept the full model of everything in my head, especially between the python and rust boundary, so I don't feel super convinced it's perfect. But I think it's probably good enough to merge and we can iterate on it in bugfix releases and anything else that comes up. Especially because this is a new function it's more important that the public interface is sustainable, and I think it looks reasonable and maintainable as a subset of the options the existing classes provided. Besides the open question about two local and other functions from @ShellyGarion I left a few small inline comments.

The only other thing I think is there are some small test coverage gaps that coveralls is flagging which would be good to exercise those paths especially as they're typical conditions for edge cases in the input which would be good to enforce the behavior of.

mtreinish · 2024-11-06T21:40:09Z

crates/accelerate/src/circuit_library/blocks.rs

+        match self {
+            Self::Standard { gate } => Ok((
+                (*gate).into(),
+                SmallVec::from_iter(params.iter().map(|&p| p.clone())),


Maybe?:

Suggested change

SmallVec::from_iter(params.iter().map(|&p| p.clone())),

SmallVec::from_iter(params.iter().cloned()),

Ah nice I didn't know about this! But it doesn't like that because it looks like from_iter expects &Param and .cloned returns Param types 🤔

But doesn't p.clone() also return a Param instance too? It's not a big deal, I kind of wrote this assuming clippy would be grumpy about it.

crates/accelerate/src/circuit_library/multi_local.rs

test/python/circuit/library/test_nlocal.py

qiskit/circuit/library/n_local/n_local.py

improve coverage & remove dangling prints

…st/nlocal

Cryoris · 2024-11-07T10:06:44Z

069f45c added tests for the flagged lines in Coveralls (for the new functions, not for the existing classes), so I think we should have the 100% on that 🙂

mtreinish

Two small inline comments, that I caught through another scan through the code. They're just small but the python side one is a potential interface issue which could cause issue depending on how things are passed in. The rust comment is more just to avoid using Box when it's not needed. I think I'm ready to approve once these are addressed

mtreinish · 2024-11-07T10:28:19Z

crates/accelerate/src/circuit_library/multi_local.rs

+    let mut packed_insts: Box<dyn Iterator<Item = PyResult<Instruction>>> =
+        Box::new((0..reps).flat_map(|layer| {
+            rotation_layer(
+                py,
+                num_qubits,
+                rotation_blocks,
+                ledger.get_parameters(LayerType::Rotation, layer),
+                &skipped_qubits,
+            )
+            .chain(maybe_barrier.get())
+            .chain(entanglement_layer(
+                py,
+                entanglement.get_layer(layer),
+                entanglement_blocks,
+                ledger.get_parameters(LayerType::Entangle, layer),
+            ))
+            .chain(maybe_barrier.get())
+        }));
+    if !skip_final_rotation_layer {
+        packed_insts = Box::new(packed_insts.chain(rotation_layer(
+            py,
+            num_qubits,
+            rotation_blocks,
+            ledger.get_parameters(LayerType::Rotation, reps),
+            &skipped_qubits,
+        )))
+    }
+
+    CircuitData::from_packed_operations(py, num_qubits, 0, packed_insts, Param::Float(0.0))


You don't need a box here, you can do something like:

Suggested change

let mut packed_insts: Box<dyn Iterator<Item = PyResult<Instruction>>> =

Box::new((0..reps).flat_map(|layer| {

rotation_layer(

py,

num_qubits,

rotation_blocks,

ledger.get_parameters(LayerType::Rotation, layer),

&skipped_qubits,

)

.chain(maybe_barrier.get())

.chain(entanglement_layer(

py,

entanglement.get_layer(layer),

entanglement_blocks,

ledger.get_parameters(LayerType::Entangle, layer),

))

.chain(maybe_barrier.get())

}));

if !skip_final_rotation_layer {

packed_insts = Box::new(packed_insts.chain(rotation_layer(

py,

num_qubits,

rotation_blocks,

ledger.get_parameters(LayerType::Rotation, reps),

&skipped_qubits,

)))

}

CircuitData::from_packed_operations(py, num_qubits, 0, packed_insts, Param::Float(0.0))

let packed_insts = (0..reps).flat_map(|layer| {

rotation_layer(

py,

num_qubits,

rotation_blocks,

ledger.get_parameters(LayerType::Rotation, layer),

&skipped_qubits,

)

.chain(maybe_barrier.get())

.chain(entanglement_layer(

py,

entanglement.get_layer(layer),

entanglement_blocks,

ledger.get_parameters(LayerType::Entangle, layer),

))

.chain(maybe_barrier.get())

});

if !skip_final_rotation_layer {

let packed_insts = packed_insts.chain(rotation_layer(

py,

num_qubits,

rotation_blocks,

ledger.get_parameters(LayerType::Rotation, reps),

&skipped_qubits,

));

CircuitData::from_packed_operations(py, num_qubits, 0, packed_insts, Param::Float(0.0))

} else {

CircuitData::from_packed_operations(py, num_qubits, 0, packed_insts, Param::Float(0.0))

}

The trick is to avoid the dynamic type when you call from_packed_operations

mtreinish · 2024-11-07T10:48:56Z

qiskit/circuit/library/n_local/n_local.py

+    if isinstance(entanglement, str):
+        return [entanglement] * num_entanglement_blocks
+
+    elif isinstance(entanglement, Iterable):


So checking isinstance Iterable isn't really the ideal way to check if something is actually iterable. It only checks if the class has been registered as an Iterable using the abc mechanisms (either as a subclass or manual registration). But python will let you iterate over some objects if __getitem__ is implemented for sequences. The only surefire way to detect if something is actually iterable is to try running iter().

We can just turn this around and use not callable(entanglement) as check, then the only valid type would be the Iterable 🙂

It's a broader thing there are a few places where isinstance(..., Iterable) is called and they'd all suffer from this same issue.

NLocal in rust -- first version

2bc79f1

Cryoris added performance Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Oct 11, 2024

fix check for _standard_gate

8649036

ShellyGarion mentioned this pull request Oct 11, 2024

Circuit library modernization #13046

Open

mtreinish added this to the 1.3.0 milestone Oct 15, 2024

Cryoris added 6 commits October 15, 2024 21:30

clean up the entanglement

3b3d69c

add n-local family members

b1a7f49

missing Mr EvolvedOperatorAnsatz and Ms QAOAAnsatz

add pending deprecations

de4f3b6

Merge branch 'main' into rust/nlocal

5501bc5

update docstrings

2eea43e

add tests

dbfe5f0

Cryoris marked this pull request as ready for review October 17, 2024 13:33

Cryoris requested review from alexanderivrii, ShellyGarion and a team as code owners October 17, 2024 13:33

mtreinish self-assigned this Oct 18, 2024

Cryoris added 3 commits October 25, 2024 16:21

fix docs

2b84ece

knowing how to read helps

a69a780

attempt no. 1 to fix the docs

d14d584

mtreinish reviewed Oct 30, 2024

View reviewed changes

Cryoris added 3 commits October 31, 2024 13:32

review comments

5fc47ee

Merge branch 'main' into rust/nlocal

9da7087

Merge branch 'main' into rust/nlocal

49a1af9

ShellyGarion added the Changelog: New Feature Include in the "Added" section of the changelog label Nov 5, 2024

ShellyGarion reviewed Nov 6, 2024

View reviewed changes

ShellyGarion mentioned this pull request Nov 6, 2024

Evolved operator functions #13361

Merged

mtreinish reviewed Nov 6, 2024

View reviewed changes

Cryoris added 2 commits November 7, 2024 11:03

review comments

069f45c

improve coverage & remove dangling prints

Merge branch 'rust/nlocal' of github.com:Cryoris/qiskit-terra into ru…

54b487c

…st/nlocal

mtreinish reviewed Nov 7, 2024

View reviewed changes

Cryoris added 2 commits November 7, 2024 12:00

remove box and fix typecheck

6950476

remove Iterable instancechecks

0f690f3

mtreinish approved these changes Nov 7, 2024

View reviewed changes

mtreinish enabled auto-merge November 7, 2024 13:05

mtreinish added this pull request to the merge queue Nov 7, 2024

Merged via the queue into Qiskit:main with commit df9ecfe Nov 7, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oxidize `NLocal` & family #13310

Oxidize `NLocal` & family #13310

Cryoris commented Oct 11, 2024 •

edited

Loading

coveralls commented Oct 11, 2024 •

edited

Loading

qiskit-bot commented Oct 17, 2024

mtreinish Oct 30, 2024

Cryoris Oct 31, 2024 •

edited

Loading

mtreinish Nov 5, 2024

Cryoris Nov 6, 2024

mtreinish Oct 30, 2024

mtreinish Oct 30, 2024

Cryoris Oct 31, 2024

mtreinish Nov 6, 2024

Cryoris Nov 7, 2024

ShellyGarion left a comment

mtreinish Nov 6, 2024

Cryoris Nov 7, 2024

mtreinish Nov 6, 2024

mtreinish left a comment

mtreinish Nov 6, 2024

Cryoris Nov 7, 2024

mtreinish Nov 7, 2024

Cryoris commented Nov 7, 2024

mtreinish left a comment

mtreinish Nov 7, 2024

mtreinish Nov 7, 2024

Cryoris Nov 7, 2024

mtreinish Nov 7, 2024

		@@ -49,6 +54,18 @@
		from test import QiskitTestCase # pylint: disable=wrong-import-order


		class Gato(Gate):

	SmallVec::from_iter(params.iter().map(\|&p\| p.clone())),
	SmallVec::from_iter(params.iter().cloned()),

Oxidize NLocal & family #13310

Oxidize NLocal & family #13310

Conversation

Cryoris commented Oct 11, 2024 • edited Loading

Summary

Details and comments

To do

coveralls commented Oct 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11723083102

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

qiskit-bot commented Oct 17, 2024

Choose a reason for hiding this comment

Cryoris Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cryoris commented Nov 7, 2024

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oxidize `NLocal` & family #13310

Oxidize `NLocal` & family #13310

Cryoris commented Oct 11, 2024 •

edited

Loading

coveralls commented Oct 11, 2024 •

edited

Loading

Cryoris Oct 31, 2024 •

edited

Loading