Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use pauli_rep to compute qml.matrix() whenever possible #5392

Closed
wants to merge 36 commits into from

Conversation

Qottmann
Copy link
Contributor

@Qottmann Qottmann commented Mar 15, 2024

Use pauli_rep whenever possible to compute the matrix of a Sum operator more efficiently.

Builds on / is blocked by #5301

[sc-58850]

@trbromley
Copy link
Contributor

Do we have any data on how much faster this is? Maybe we can use @obliviateandsurrender's benchmark.

@Qottmann Qottmann marked this pull request as ready for review March 26, 2024 17:01
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@Qottmann Qottmann changed the title [WIP] Use pauli_rep to compute Sum.matrix() more efficiently Use pauli_rep to compute Sum.matrix() and Prod.matrix() more efficiently Mar 26, 2024
@Qottmann
Copy link
Contributor Author

Some numbers, the speedup is actually not really significant 🤔

op = qml.sum(*(0.5 * (X(i) @ X(i+1)) for i in range(10)))
op += qml.sum(*(0.7 * (Z(i) @ Y(i+1)) for i in range(10)))
op += qml.sum(*(1.5 * (Z(i) @ Y(i+3)) for i in range(10)))

%timeit a = op.matrix() # this branch
2.41 s ± 31.3 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit a = op.matrix() # master
2.84 s ± 168 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

@Qottmann
Copy link
Contributor Author

Qottmann commented Apr 4, 2024

qml.matrix(Hamiltonian) is fast because it is using the sparse_matrix method. I just pushed an upgrade that also enables using that for Sum and Prod instances with some considerable speed - up :)

op = qml.sum(*(0.5 * (X(i) @ X(i+1)) for i in range(10)))
op += qml.sum(*(0.7 * (Z(i) @ Y(i+1)) for i in range(10)))
op += qml.sum(*(1.5 * (Z(i) @ Y(i+3)) for i in range(10)))

%timeit a = qml.matrix(op) # this branch
109 ms ± 8.92 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit a = qml.matrix(op) # master branch
3.65 s ± 176 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
import pyscf
import numpy as np
h3_molecule = [["H", (0, 0, 0)], ["H", (0, 1.2, 1.2)], ["H", (0, 0, 2.4)]]
molecule, basis, symm, charge, spin, tol = h3_molecule, "631g", None, 0, 1, 1e-6
mol = pyscf.gto.M(atom=molecule, basis=basis, symmetry=symm, charge=charge, spin=spin)
myhf = pyscf.scf.ROHF(mol).run(verbose=0)
core_constant = np.array([mol.energy_nuc()])
# molecular integrals in AO basis
one_ao = mol.intor_symmetric("int1e_kin") + mol.intor_symmetric("int1e_nuc")
two_ao = mol.intor("int2e_sph")
# rotate to MO basis
one_mo = np.einsum("pi,pq,qj->ij", myhf.mo_coeff, one_ao, myhf.mo_coeff)
two_mo = pyscf.ao2mo.incore.full(two_ao, myhf.mo_coeff)
# physicist ordering convention
two_mo = np.swapaxes(two_mo, 1, 3)
h_ferm = qml.qchem.fermionic_observable(core_constant, one_mo, two_mo)

with qml.operation.disable_new_opmath_cm():
    H = qml.qchem.qubit_observable(h_ferm)
    print(len(H.terms()[1]), type(H))
    %timeit qml.matrix(H)

with qml.operation.enable_new_opmath_cm():
    H = qml.qchem.qubit_observable(h_ferm)
    print(len(H.terms()[1]), type(H))
    %timeit qml.matrix(H)
923 <class 'pennylane.ops.qubit.hamiltonian.Hamiltonian'>
1.15 s ± 17.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
923 <class 'pennylane.ops.op_math.sum.Sum'>
139 ms ± 914 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

@Qottmann Qottmann changed the base branch from master to pauli-swap-order April 4, 2024 15:25
@Qottmann Qottmann requested a review from a team as a code owner April 4, 2024 15:25
@albi3ro
Copy link
Contributor

albi3ro commented Apr 4, 2024

qml.matrix(Hamiltonian) is fast because it is using the sparse_matrix method. I just pushed an upgrade that also enables using that for Sum and Prod instances with some considerable speed - up :)

The issue is that Operator.sparse_matrix is not differentiable, but Operator.matrix() can be, so I would say switching to use the sparse matrix instead of the dense matrix to be a feature regression. We used Hamiltonian.sparse_matrix because we never got around to adding Hamiltonian.matrix, so we had no alternative.

I do think we could leverage PauliWord._to_sparse_matrix to improve the performance of calculating a dense matrix, but we need to make sure that dense matrix continues to be fully differentaible.

@Qottmann
Copy link
Contributor Author

Qottmann commented Apr 5, 2024

I am using the pauli_rep.to_matrix() function now but getting some unexpected failures in other parts of the codebase (e.g. diagonalize_qwc_pauli_words). Will have to dig in later

Edit: The failures seem to come from #5301 This PR is currently blocked by this

@trbromley
Copy link
Contributor

There's currently a CI check failing, but bringing in @maliasadi @vincentmr here as this is a performance issue related to op arithmetic. Thanks!

@vincentmr
Copy link
Contributor

There's currently a CI check failing, but bringing in @maliasadi @vincentmr here as this is a performance issue related to op arithmetic. Thanks!

Maybe we should bring in @mudit2812 too, since he's the instigator of #5301 .

@albi3ro
Copy link
Contributor

albi3ro commented Apr 18, 2024

Would it be possible to update PauliSentence.matrix() to take advantage of the sparse speed up of the non-differentiable PauliWord.matrix(), but maintain the differentiability of the dense matrix?

I'd like to keep the dense matrix differentiable if at all possible.

@mudit2812
Copy link
Contributor

mudit2812 commented Apr 18, 2024

Maybe we should bring in @mudit2812 too, since he's the instigator of #5301 .

I fixed all failing CI in my PR. I will update branch and let's see if CI passes. Once CI passes, I will run lightning CI against this branch as well to validate that tests pass. I'll update this PR accordingly, but if lightning tests break because of this PR, we should have a PR on the lightning side ready to be merged right after this.

@trbromley trbromley requested a review from mlxd April 19, 2024 12:37
@trbromley
Copy link
Contributor

Also keeping @mlxd in the loop 👍

@Qottmann Qottmann changed the title Use pauli_rep to compute Sum.matrix() and Prod.matrix() more efficiently Use pauli_rep to compute qml.matrix() whenever possible Apr 19, 2024
@Qottmann
Copy link
Contributor Author

In the current implementation that uses pauli_rep.to_mat(format="csr"), indeed coefficients parametrizing an operator, for which we compute the matrix via qml.matrix(), is not differentiable right now (you can still differentiate "through it" but not coefficient inside of it).

I.e.

def f(x):
    mat = qml.matrix(x * X(0))
    return jnp.sum(mat)

x = jnp.array(0.5, dtype=complex)
f(x), jax.grad(f, holomorphic=True)(x)
(Array(1.+0.j, dtype=complex128), Array(0.+0.j, dtype=complex128))

This is why the pulse tests fail (the "true" results are computed with explicit matrices evaluated via qml.matrix(), using H(params, t).matrix() would solve that)

@albi3ro I didnt fully grasp your suggestion, could you elaborate which specific steps you would take?

@trbromley trbromley added this to the v0.36 milestone Apr 19, 2024
pennylane/ops/op_math/prod.py Outdated Show resolved Hide resolved
pennylane/ops/op_math/sum.py Outdated Show resolved Hide resolved
@@ -211,8 +211,10 @@ def circuit():
if isinstance(op, qml.operation.Tensor) and wire_order is not None:
op = 1.0 * op # convert to a Hamiltonian

if isinstance(op, qml.ops.Hamiltonian):
if (pr := op.pauli_rep) is not None:
return pr.to_mat(wire_order=wire_order or op.wires, format="csr").toarray()
Copy link
Member

Choose a reason for hiding this comment

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

Is CSR what we want to use for this?

Copy link
Contributor Author

@Qottmann Qottmann Apr 24, 2024

Choose a reason for hiding this comment

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

For context, this came up in switching to new opmath because some generators and qchem operators where now slower in computing qml.matrix(), as they were no longer ops.Hamiltonian instances and therefore didnt hit the op.sparse_matrix(..).toarray() method below anymore. The idea was to just allow still doing that via the pauli rep in this PR; but as @albi3ro pointed out this breaks differentiability. Afaik christina has been cooking up an alternate solution 👩‍🍳

Base automatically changed from pauli-swap-order to master April 23, 2024 19:06
@Qottmann
Copy link
Contributor Author

Closing in favor of #5578 that ensures differentiability

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.

6 participants