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

[BUG] Grouping of measurements is inconsistent between legacy and new opmath #5512

Closed
1 task done
lillian542 opened this issue Apr 12, 2024 · 1 comment · Fixed by #5525
Closed
1 task done

[BUG] Grouping of measurements is inconsistent between legacy and new opmath #5512

lillian542 opened this issue Apr 12, 2024 · 1 comment · Fixed by #5525
Labels
bug 🐛 Something isn't working

Comments

@lillian542
Copy link
Contributor

lillian542 commented Apr 12, 2024

Expected behavior

That measurements=[qml.expval(qml.PauliX(0)), qml.expval(qml.PauliY(0)@qml.PauliX(1)] would be grouped the same and be tracked as requiring the same number of executions to measure regardless of whether the observables were Tensor or Prod instances.

Actual behavior

There is a separate clause for grouping new opmath instances that causes mathematically identical measurements to be grouped differently. The legacy behaviour seems preferable and results in fewer executions.

Additional information

The issue arises in pennylane.device.qubit.sampling._group_measurements. It was found in a test for the tracker that passes with Legacy opmath but not with the new opmath, but it appears to be genuinely increasing the number of executions run, not just incorrectly counting in the tracker.

There is an xfailed test in test_default_qubit_tracker.py that should pass (i.e. fail due to strict xfail) once this is resolved.

Source code

>>> from pennylane.devices.qubit.sampling import get_num_shots_and_executions, _group_measurements

>>> prod = qml.prod(qml.PauliX(0), qml.PauliY(1))
>>> tensor = qml.operation.Tensor(qml.PauliX(0), qml.PauliY(1))

>>> tape1 = qml.tape.QuantumScript([], measurements=[qml.expval(qml.PauliX(0)), qml.expval(prod)], shots=10)
>>> get_num_shots_and_executions(tape1)
(2, 20)

>>> all_mp_groups, all_indices = _group_measurements(tape1.measurements)
>>> all_mp_groups
[[expval(X(0))], [expval(X(0) @ Y(1))]]


>>> tape2 = qml.tape.QuantumScript([], measurements=[qml.expval(qml.PauliX(0)), qml.expval(tensor)], shots=10)
>>> get_num_shots_and_executions(tape2)
(1, 10)

>>> all_mp_groups, all_indices = _group_measurements(tape2.measurements)
>>> all_mp_groups
[[expval(X(0)), expval(X(0) @ Y(1))]]

Tracebacks

No response

System information

pl-dev

Existing GitHub issues

  • I have searched existing GitHub issues to make sure the issue does not already exist.
@lillian542 lillian542 added the bug 🐛 Something isn't working label Apr 12, 2024
@trbromley
Copy link
Contributor

trbromley commented Apr 15, 2024

Thanks @lillian542!

The legacy behaviour seems preferable and results in fewer executions.

This looks like a priority to fix before the 0.36 release.

lillian542 added a commit that referenced this issue Apr 17, 2024
Currently, the tests only run with the new opmath, so won't know if we
introduced a bug that breaks the legacy opmath behaviour while it is
still in its deprecation cycle.

This PR creates a kwarg, `disable-opmath`, that can be passed when
running the tests, and a corresponding workflow that runs tests with
that kwarg set to True. This can also be used locally, i.e. `python -m
pytest tests/ --disable-opmath=True`

Right now, it runs with CI on the PR, but once the tests are all
passing, the line triggering that will be removed and it will only run
every 3-4 days, in the middle of the night. Then we will add it to the
test matrix (separate PR to modify the plugin test matrix repo).

The changes to .yml files and to the `conftest` files are all about
allowing us to run these additional tests. A few modifications to tests
were made to allow them to pass with both legacy opmath and new opmath.

There is one test currently marked as xfail for new opmath that I would
call a bug - I opened an issue here:
#5512

---------

Co-authored-by: Mudit Pandey <[email protected]>
Co-authored-by: qottmann <[email protected]>
astralcai added a commit that referenced this issue Apr 21, 2024
…miltonian (#5525)

**Context:**
The `_group_measurements` function in `qubit/sampling.py` groups
measurement processes with commuting observables together to reduce the
number of executions. Now it does not handle `Prod`, `SProd` or
single-term `Hamiltonian` and `Sum`, which leads to inefficiencies with
new opmath.

**Description of the Change:**
Removes the branch that places `Prod`, `SProd`, `Hamiltonian` and `Sum`
in separate groups. Now `is_pauli_word` is used for everything.

**Benefits:**
Better grouping leads to fewer number of executions.

**Related GitHub Issues:**
Fixes #5512
[sc-61306]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants