Accept NumPy scalar coefficients across operator and representation types#1361
Conversation
|
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. |
There was a problem hiding this comment.
Code Review
This pull request enables support for NumPy scalar coefficients across several operator and Hamiltonian classes (such as MajoranaOperator, DOCIHamiltonian, and PolynomialTensor) by adding numbers.Number to the allowed coefficient types, alongside adding corresponding tests. The review feedback suggests optimizing the multiplication operators in MajoranaOperator by avoiding dynamic tuple unpacking in isinstance checks to prevent performance overhead. Additionally, the reviewer points out that several new test cases were incorrectly defined as standalone functions instead of methods within unittest.TestCase classes, which would cause them to be skipped by standard test runners.
|
Thanks for the review. Both points are addressed in 91517da:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates several operator and representation classes, including majorana_operator, DOCIHamiltonian, and PolynomialTensor, to accept NumPy scalar coefficients by including numbers.Number in their coefficient type checks. It also adds corresponding unit tests to verify that NumPy scalar coefficients behave identically to Python scalars. There are no review comments, so I have no feedback to provide.
mhucka
left a comment
There was a problem hiding this comment.
Thank you for working on this. I had a couple of minor change requests; otherwise, it's looking good.
|
|
||
| def __mul__(self, other): | ||
| if not isinstance(other, (type(self), int, float, complex)): | ||
| if not isinstance(other, (type(self), COEFFICIENT_TYPES)): |
There was a problem hiding this comment.
This tuple is constructed on every call in a couple of places in this class. It would be slightly more efficient and more readable to define a tuple for this,
_MAJORANA_MUL_TYPES = (MajoranaOperator, int, float, complex, numbers.Number)and then use it in place of (type(self), COEFFICIENT_TYPES).
There was a problem hiding this comment.
Done in 7942bbe. Added a module-level _MAJORANA_MUL_TYPES = (MajoranaOperator,) + COEFFICIENT_TYPES, built once after the class, and used it in both __mul__ and __imul__.
|
|
||
| import numpy | ||
|
|
||
| COEFFICIENT_TYPES = (int, float, complex, numbers.Number) |
There was a problem hiding this comment.
This definition of COEFFICIENT_TYPES is repeated in multiple files. An approach to avoiding this, to keep the code more DRY and maintainable, would be to define this constant in one file (namely src/openfermion/config.py), like this:
import numbers
COEFFICIENT_TYPES = (int, float, complex, numbers.Number)and change the other files to do the equivalent of the following:
import openfermion.config as config
COEFFICIENT_TYPES = config.COEFFICIENT_TYPESDoing it this way
-
is more efficient than referencing
config.COEFFICIENT_TYPESmultiiple times all over the code, because it avoids Python having to do multiple symbol table lookups -
is in keeping with the project principles of not importing individual symbols from modules (i.e., not doing
from openfermion.config import COEFFICIENT_TYPES.
Can you make the appropriate changes? (Note that the imports may need to be moved to the appropriate place in the imports portion of the files, not put literally right before the uses as I showed above.)
There was a problem hiding this comment.
Done in 7942bbe. Moved it to openfermion/config.py; the other modules now do import openfermion.config as config and bind COEFFICIENT_TYPES = config.COEFFICIENT_TYPES locally. I left symbolic_operator.py as-is since its version also includes sympy.Expr and isn't part of this PR, but I can fold it in too if you want.
|
Thanks, both done in 7942bbe.
One thing I left alone: |
| FermionOperator | ||
| """ | ||
| if not isinstance(coefficient, (int, float, complex)): | ||
| if not isinstance(coefficient, config.COEFFICIENT_TYPES): |
There was a problem hiding this comment.
I think the latest changes missed this one. The reference to config.COEFFICIENT_TYPES can be pulled up to an assignment at the module level, like you did in majorana_operators.py
There was a problem hiding this comment.
Good catch, done in 9dc03f2. Pulled it up to a module-level COEFFICIENT_TYPES = config.COEFFICIENT_TYPES in special_operators.py to match the other files.
…ypes NumPy scalar types (numpy.int64, numpy.float32, numpy.complex64, and so on) are not subclasses of Python's int/float/complex, so coefficient checks of the form isinstance(x, (int, float, complex)) reject them. That is the root of issue quantumlib#1097. The SymbolicOperator family (FermionOperator, QubitOperator, ...) was already fixed by adding numbers.Number to its accepted coefficient types; this completes the same fix for the remaining types that validate coefficients the same way: - PolynomialTensor (and InteractionOperator and friends) - DOCIHamiltonian - MajoranaOperator - the majorana_operator builder in special_operators numbers.Number matches Python and NumPy numeric scalars alike, so an accepted coefficient now behaves identically whether it comes from Python or NumPy. Adds a test in each affected module checking that NumPy scalar coefficients give the same result as their Python equivalents. Fixes quantumlib#1097.
…e classes Two changes in response to the automated review on this PR: - MajoranaOperator.__mul__ and __imul__ unpacked COEFFICIENT_TYPES into a fresh tuple on every call via (type(self), *COEFFICIENT_TYPES). Switched to a single isinstance with a nested tuple, (type(self), COEFFICIENT_TYPES), which avoids the per-call unpacking and stays a single isinstance call (so pylint's consider-merging-isinstance check remains satisfied). - The three new NumPy-scalar tests were module-level functions in files whose other tests live in unittest.TestCase classes, so they would be skipped under plain unittest discovery. Moved them into PolynomialTensorTest, DOCIHamiltonianTest, and MajoranaOperatorTest and switched to self.assertEqual.
Addresses the review comments on this PR: - COEFFICIENT_TYPES was defined identically in several files. Moved the definition to openfermion/config.py and have the other modules pick it up via `import openfermion.config as config` plus a local COEFFICIENT_TYPES = config.COEFFICIENT_TYPES binding, so there is a single source of truth and no repeated attribute lookups. While editing polynomial_tensor.py I routed its EQ_TOLERANCE through the same config import so the file does not mix import styles; special_operators.py uses config.COEFFICIENT_TYPES directly for its one check. - MajoranaOperator.__mul__ and __imul__ built (type(self), COEFFICIENT_TYPES) on every call. Added a module-level _MAJORANA_MUL_TYPES tuple, built once after the class, and use it in both.
special_operators.py still referenced config.COEFFICIENT_TYPES inline. Pulled it up to a module-level COEFFICIENT_TYPES = config.COEFFICIENT_TYPES binding to match majorana_operator.py and the representation modules.
9dc03f2 to
a33759a
Compare
mhucka
left a comment
There was a problem hiding this comment.
Thanks for hanging in there! This looks good. Going to merge it now.
Fixes #1097.
The actual problem behind #1097 is that NumPy's scalar types aren't subclasses of Python's
int,float, orcomplex.numpy.int64,numpy.float32,numpy.complex64and the rest all fail anisinstance(x, (int, float, complex))check, which is the idiom the coefficient validation uses in a few places. (The one that slips through isnumpy.float64, since it does subclassfloat, which is probably why this went unnoticed for a while.)The
SymbolicOperatorfamily already got fixed by addingnumbers.Numberto its accepted types, soFermionOperator,QubitOperatorand so on are fine now. But the same(int, float, complex)pattern shows up in other spots that the change didn't reach, and those still reject NumPy scalars:PolynomialTensor(soInteractionOperator,InteractionRDM, ...)DOCIHamiltonianMajoranaOperatormajorana_operatorbuilder inspecial_operators.pyThis adds
numbers.Numberto each of them, matching the existing fix.numbers.Numbercovers Python and NumPy numeric scalars equally, so a coefficient behaves the same whether it came from Python or NumPy, and the plain int/float/complex cases stay exactly as they were.One thing worth calling out: dividing or multiplying a real-dtype tensor by a complex scalar still raises, because NumPy won't do the in-place cast from complex to float. That happens with a plain Python
complextoo, so it's pre-existing and unrelated to this change, and I left it alone.Each affected module gets a test asserting that NumPy scalar coefficients (
int64,float32,complex64) give the same result as the equivalent Python scalar, including the in-place operators onMajoranaOperator. The tests fail onmainand pass with this change.