Add flatten option to the NLocal family

[mtreinish]

Summary

This commit adds a new flag flatten to the circuit library elements in the NLocal family. This flag is used to avoid the nested wrapping of the output that is done by default in the output of these gates. This is done primarily in the interest of performance. The runtime performance of bind_parameters() is vastly improved with a flattened output compared to the nested object. For example, running:

qc = EfficientSU2(100, entanglement="linear", reps=100)
qc.measure_all()
qc.bind_parameters({x: math.pi / 2 for x in qc.parameters})

the runtime of bind_parameters() went from ~390 seconds with the default wrapped output to ~0.5 seconds with the flattened output. I think given these results longer term we might want to flip the default behavior to only wrap on user request (likely for visualization as that's the only use case I can think of for the wrapped output). The default value is set to None in this PR to facilitate this change. In 0.26/0.45 we can emit a warning if flatten is None to warn that the default will change in a future release.

Details and comments

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Cryoris
• @Qiskit/terra-core
• @ajavadia

[woodsp-ibm]

A couple of thoughts:

Would it make sense to expose flatten as a property too so that you can change how it comes out for visualization without having to construct a whole new identical circuit? In notebooks we show an ansatz (or a combination of feature map and anstaz in ML) - sometimes decomposed at present sometimes not depending on what we want to show. Changing flatten would invalidate it so it needs to get built again.

I wonder whether this could/should be done for data_preparation (feature maps) too.

QAOAnsatz is not directly an NLocal cct but goes through EvolvedOperatorAnsatz (which UCC in Nature is based on). I saw that was not changed but further see the tests with qaoa failed around passing flatten to the parent.

[mrossinek]

Also an FYI from my side: personally, I always decompose my ansatz multiple times even for visualization as I don't find the "non-flattened" structure useful at all.
Ofcourse this makes things potentially massive but I tend to prefer it this way (just as my 2 cents).

Also: in the past I ran into an issue when storing a subclass of an EvolvedOperatorAnsatz as QASM and later trying to load it. It would break because of some custom operations being defined in QASM. I am not sure whether this ever got resolved because I ended up decomposing the ansatz again multiple times to work around this problem.

And speaking of the EvolvedOperatorAnsatz: I think this should get the same flatten argument since its a subclass of NLocal, too.

[mtreinish]

A couple of thoughts:

Would it make sense to expose flatten as a property too so that you can change how it comes out for visualization without having to construct a whole new identical circuit? In notebooks we show an ansatz (or a combination of feature map and anstaz in ML) - sometimes decomposed at present sometimes not depending on what we want to show. Changing flatten would invalidate it so it needs to get built again.

I worry about changing the contents of the circuit dynamically like this if feels kind of error prone (granted I feel that way about blueprint circuits in general though). Maybe it's as easy as setting _is_built to false in the setter. I'll test it locally

I wonder whether this could/should be done for data_preparation (feature maps) too.

Yeah I started small here, but I think we'll want to do this to all the circuit library classes that have wrapped output. The performance difference is so drastically improved. I was tempted to just revert #6634 at first. But figured this approach was a bit more sustainable than a breaking api change revert of a breaking change from 2 yrs ago.

QAOAnsatz is not directly an NLocal cct but goes through EvolvedOperatorAnsatz (which UCC in Nature is based on). I saw that was not changed but further see the tests with qaoa failed around passing flatten to the parent.

Yeah, I reverted this when I saw. I was just confused because it's in the nlocal directory and was kind of on autopilot when I was making the changes.

[woodsp-ibm]

Maybe it's as easy as setting _is_built to false in the setter. I'll test it locally

I believe we call self._invalidate() normally on a property update that requires the circuit to be re-built.

[jakelishman]

Max: could you link the circuit in an issue, if you find it, or see if it's a duplicate of #10162? (I suspect it might be the latter.)

Matthew's investigation made me realise that QuantumCircuit._rebind_definition may well predate some changes to Terra's data model, since it seems to behave as if Instruction._definition could still have been a list (i.e. it doesn't check global_phase and it doesn't recurse the way I'd expect). I'm just going to try out a little tweak to its recursion, which will coincidentally fix the problem of a parametrised global phase as well, and see what sorts of performance updates we get.

Fwiw, I think there are at least a couple of algorithms-y objects that involve two layers of wrapping an object (though off the top of my head, I can't remember what they are) when they were only intended to be one layer.

[ShellyGarion]

I think that also EvolvedOperatorAnsatz belongs to the Nlocal family. See: #8326

[coveralls]

Pull Request Test Coverage Report for Build 5590876157

• 21 of 23 (91.3%) changed or added relevant lines in 3 files are covered.
• 15 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.001%) to 86.048%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/library/n_local/n_local.py	15	17	88.24%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
crates/qasm2/src/lex.rs	2	91.14%
crates/qasm2/src/parse.rs	12	97.11%

Totals
Change from base Build 5589940418:	0.001%
Covered Lines:	72400
Relevant Lines:	84139

---

💛 - Coveralls

[Cryoris]

This is a good addition 👍🏻 But the current implementation doesn't quite do what I expected from the documentation. For example, the EfficientSU2 nicely gets decomposed in standard gates:

>>> EfficientSU2(3, flatten=True, reps=1).draw()
     ┌──────────┐┌──────────┐                  ┌──────────┐ ┌──────────┐
q_0: ┤ Ry(θ[0]) ├┤ Rz(θ[3]) ├──────────■───────┤ Ry(θ[6]) ├─┤ Rz(θ[9]) ├
     ├──────────┤├──────────┤        ┌─┴─┐     ├──────────┤┌┴──────────┤
q_1: ┤ Ry(θ[1]) ├┤ Rz(θ[4]) ├──■─────┤ X ├─────┤ Ry(θ[7]) ├┤ Rz(θ[10]) ├
     ├──────────┤├──────────┤┌─┴─┐┌──┴───┴───┐┌┴──────────┤└───────────┘
q_2: ┤ Ry(θ[2]) ├┤ Rz(θ[5]) ├┤ X ├┤ Ry(θ[8]) ├┤ Rz(θ[11]) ├─────────────
     └──────────┘└──────────┘└───┘└──────────┘└───────────┘

However, more complex circuits, such as the EvolvedOperatorAnsatz don't seem "flattened" but just less convolved:

>>> EvolvedOperatorAnsatz(op, flatten=True).draw()
     ┌─────────────────────────────┐
q_0: ┤0                            ├
     │                             │
q_1: ┤1 exp(-it (ZZZ + XXX))(t[0]) ├
     │                             │
q_2: ┤2                            ├
     └─────────────────────────────┘

For an argument called "flatten" I would rather expect something that decomposes to standard gates, no matter the nesting:

# on this circuit, h rz cx are enough, but this should include all standard gates
>>> transpile(EvolvedOperatorAnsatz(op), basis_gates=["h", "rz", "cx"]).draw() 
          ┌───┐┌──────────────┐┌───┐┌───┐          ┌───┐┌──────────────┐┌───┐┌───┐
q_0: ─────┤ X ├┤ Rz(2.0*t[0]) ├┤ X ├┤ H ├──────────┤ X ├┤ Rz(2.0*t[0]) ├┤ X ├┤ H ├─────
     ┌───┐└─┬─┘└──────────────┘└─┬─┘├───┤┌───┐┌───┐└─┬─┘└──────────────┘└─┬─┘├───┤┌───┐
q_1: ┤ X ├──■────────────────────■──┤ X ├┤ H ├┤ X ├──■────────────────────■──┤ X ├┤ H ├
     └─┬─┘                          └─┬─┘├───┤└─┬─┘                          └─┬─┘├───┤
q_2: ──■──────────────────────────────■──┤ H ├──■──────────────────────────────■──┤ H ├
                                         └───┘                                    └───┘

That's analog to what numpy.flatten does -- no matter the dimension of your tensor, a flattened array is a vector.

So if this argument is supposed to "flatten", would it be better to decompose to the standard gates? Otherwise we could also rename it to wrap (default=True), to make it clearer?

[mtreinish]

So if this argument is supposed to "flatten", would it be better to decompose to the standard gates? Otherwise we could also rename it to wrap (default=True), to make it clearer?

I fixed the flatten behavior for EvolvedOperatorAnsatz here: b56e960

The tricky bit for that class was that it was built using other circuit library elements which were proper gates, so from the perspective of the flatten option it was flattened to just gate objects (instead of wrapped circuits containing other circuit library elements).

[Cryoris]

LGTM, just tiny comments on consistency on the signature that would be nice to get in, but not required.

[Cryoris]

For consistency with the rest of the signature (also the Optional import would need to be removed 🙂)

Suggested change

	flatten: Optional[bool] = None,
	flatten: bool | None = None,

[Cryoris]

Suggested change

	flatten: Optional[bool] = None,
	flatten: bool | None = None,

[Cryoris]

Suggested change

	flatten: Optional[bool] = None,
	flatten: bool | None = None,

[mtreinish]

In the interest of time I've enqueued it to merge I'll open an issue based on your comment and track that as a fix post-rc1 (since it's just a doc change)