Replace assert in reverse_jordan_wigner.py#1257
Replace assert in reverse_jordan_wigner.py#1257mhucka wants to merge 10 commits intoquantumlib:mainfrom
Conversation
Replace another `assert` in a non-test context.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces an assertion with an explicit ValueError in the reverse Jordan-Wigner transformation to ensure the QubitOperator contains exactly one term. The review feedback suggests refining the error message for better clarity and notes that the check should ideally occur before any indexing to avoid shadowing the error with an IndexError.
mhucka
changed the title
mhucka
changed the title
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This tests the corresponding change in reverse_jordan_wigner.py
| def test_reverse_jw_multi_term_error(self): | ||
| with patch( | ||
| 'openfermion.transforms.opconversions.reverse_jordan_wigner.' 'QubitOperator.__mul__' | ||
| ) as mock_mul: | ||
| mock_mul.return_value = QubitOperator('X0') + QubitOperator('Y0') | ||
| with self.assertRaisesRegex(ValueError, 'QubitOperator must contain exactly one term'): | ||
| reverse_jordan_wigner(QubitOperator('X1')) |
There was a problem hiding this comment.
why do you need to mock stuff?
There was a problem hiding this comment.
It's to simulate an internal failure condition. The code uses unittest.mock.patch to replace the __mul__ method of QubitOperator, specifically where it is used inside the reverse_jordan_wigner module, so that it can force any multiplication operation to return a multiterm operator (X0 + Y0) instead of a single Pauli string.
Maybe I'm missing another, easier way? (Entirely possible …)
There was a problem hiding this comment.
Naively, I would think that if you can't trigger the failure condition without mucking up the library code itself (i.e. it can't be triggered by bad user input), then I would think an assertion would be the more idiomatic way to guard this
Include info about the number of terms actually found.
|
I suspect the problem is that the function being tested should be changed somehow. Anyway, I'm going to close this because this doesn't seem worth spending more time on. |
Pull request was closed
2 participants
Replace an
assertin a non-test context with raising an exception.