Fix #880: return simplified QubitOperator from symmetry_conserving_bravyi_kitaev

[stark256-spec]

Closes #880.

Problem

get_sparse_operator raises on the operator produced by symmetry_conserving_bravyi_kitaev:

from openfermion import get_sparse_operator, FermionOperator, symmetry_conserving_bravyi_kitaev

fermion_op = FermionOperator("0^ 1^")
qubit_op = symmetry_conserving_bravyi_kitaev(fermion_op, 4, 2)
get_sparse_operator(qubit_op)   # ValueError: axis 0 index 7 exceeds matrix dimension 4

Root cause

symmetry_conserving_bravyi_kitaev finishes by calling remove_indices, which shifts qubit indices. When two qubits are mapped onto the same new index, a term ends up with multiple Paulis acting on one qubit, e.g.:

((0, X), (1, Y), (1, X))

remove_indices writes these terms straight into the operator’s .terms dict, so they bypass the simplification QubitOperator normally performs on construction. qubit_operator_sparse assumes each qubit appears at most once per term (it grows the tensor product one factor per Pauli), so a repeated qubit makes the per-term matrix larger than the n_qubits Hilbert space and the assembly raises.

Fix

Rebuild the operator after remove_indices so every term is routed back through QubitOperator’s simplification, restoring canonical one-Pauli-per-qubit form (e.g. ((0, X), (1, Y), (1, X)) → ((0, X), (1, Z))). This keeps the operator mathematically identical — the existing eigenspectrum-based tests still pass — while satisfying the invariant that QubitOperators are simplified, which get_sparse_operator and other consumers rely on.

Tests

Added test_output_is_simplified_qubit_operator, a regression test built from the reporter's reproducer. It now asserts the transform equals an independently worked-out, fully-simplified QubitOperator for that input (rather than re-deriving the expectation from the output), checks every term is canonical (one Pauli per qubit), and checks get_sparse_operator no longer raises. The existing eigenspectrum-based tests continue to pass, confirming the operator is unchanged mathematically.

Verified with the repo's check scripts: check/pytest -m "not slow" src/openfermion/transforms/opconversions/remove_symmetry_qubits_test.py passes, and check/format-incremental and check/pylint-changed-files are clean. check/mypy reports no issues in the changed module.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request addresses issue #880 by ensuring that symmetry_conserving_bravyi_kitaev returns a simplified QubitOperator in canonical form (with at most one Pauli operator per qubit). This is achieved by rebuilding the operator term-by-term, which triggers QubitOperator's internal simplification and prevents errors when calling get_sparse_operator. A regression test has also been added to verify this behavior. There are no review comments, so I have no additional feedback to provide.

[mhucka]

Thank you for this work. This is almost ready; I just have one trivial change and a question about one of the tests.

[mhucka]

Should this use integer division instead?

Suggested change

	qubit_hamiltonian = remove_indices(qubit_hamiltonian, (active_orbitals / 2, active_orbitals))
	qubit_hamiltonian = remove_indices(qubit_hamiltonian, (active_orbitals // 2, active_orbitals))

[mhucka]

I'm not 100% sure, but I think this does not achieve the objective of the test. The problem is that trafo_op is already simplified, if I understand what symmetry_conserving_bravyi_kitaev is doing. So, the expected here is being constructed from an already-simplified thing. which means the comparison on line 179 of sparse_op to expected essentially compares trafo_op against itself.

[mhucka]

Also, @stark256-spec, the testing process described in this PR's description are not what this project does. Please see CONTRIBUTING.md for an explanation of what to do.

[stark256-spec]

Thanks @mhucka — both addressed in a585261.

1. Integer division. Done. I switched both active_orbitals / 2 index computations to // — the remove_indices call you flagged, plus the sibling one on the line above that feeds edit_hamiltonian_for_spin, since it's the same index pattern. It keeps the indices integer-typed; the existing LiH/Hubbard/single-operator tests still pass unchanged. Happy to drop the second one if you'd rather keep this minimal.

2. The test — you're right, thank you. Now that the fix returns an already-simplified operator, re-summing its terms is a no-op, so the old expected was trafo_op in disguise and the assertion compared it against itself. I replaced it with an independently worked-out QubitOperator for this input and assert the transform equals it (self.assertEqual(trafo_op, expected_op)), then compare the sparse matrices. That pins the actual value rather than comparing the result to itself.

All 6 tests pass; black/pylint/mypy clean.

[stark256-spec]

Good point, thanks — I've updated the PR description's testing section to follow CONTRIBUTING.md. It now describes the regression test and references the repo's own check scripts (check/pytest -m "not slow" …, check/format-incremental, check/pylint-changed-files) instead of the ad-hoc tool invocations I'd listed before.