Skip to content

Clean up redundant complex type unions #7041

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

Merged
merged 5 commits into from
Feb 7, 2025
Merged

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented Feb 6, 2025

Fixes #2375.

In type signatures, Union[complex, float, int] and such can be simplified to complex, and this PR does so. Mypy does not work well with numbers.Complex in type signatures, so this PR does not use it there.

In isinstance checks, (complex, numbers.Complex) is always redundant, and this PR replaces it with just isinstance(numbers.Complex).

isinstance(numbers.Complex) also returns true for float and int values, whereas isinstance(complex) returns false. Sometimes in the code isinstance(complex) was just used to differentiate number from symbolic or None. I changed those to isinstance(number.Complex) for broader applicability. Sometimes, isinstance(complex) was intentionally added to exclude float; for instance float does not have .imag or .real, and for those I left as-is. A follow-up item could be to replace val.real etc. with np.real(val) in those cases, which would allow standardizing on isinstance(numbers.Complex) everywhere.

The specific bug in #2375 is fixed with an addition to PauliString.__array_ufunc__.

@daxfohl daxfohl requested review from wcourtney, vtomole, verult and a team as code owners February 6, 2025 16:11
@CirqBot CirqBot added the size: M 50< lines changed <250 label Feb 6, 2025
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.17%. Comparing base (d589bfc) to head (13d764d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7041      +/-   ##
==========================================
- Coverage   98.17%   98.17%   -0.01%     
==========================================
  Files        1085     1085              
  Lines       94672    94683      +11     
==========================================
+ Hits        92942    92952      +10     
- Misses       1730     1731       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM with two small tweaks. Thank you for hunting these down!

@@ -33,7 +33,7 @@ def all_near_zero(a: 'ArrayLike', *, atol: float = 1e-8) -> bool:


def all_near_zero_mod(
a: Union[float, complex, Iterable[float], np.ndarray], period: float, *, atol: float = 1e-8
a: Union[complex, Iterable[float], np.ndarray], period: float, *, atol: float = 1e-8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
a: Union[complex, Iterable[float], np.ndarray], period: float, *, atol: float = 1e-8
a: Union[complex, Iterable[complex], np.ndarray], period: float, *, atol: float = 1e-8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually went the opposite direction. The function fails on complex numbers (there's a mod operation in it), so I'm not sure why complex was an option at all.

def __mul__(
self, other: Union[complex, int, float, numbers.Number]
) -> 'cirq.PauliString[TKey]':
def __mul__(self, other: Union[complex, numbers.Number]) -> 'cirq.PauliString[TKey]':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am getting True for issubclass(complex, numbers.Number) -

Suggested change
def __mul__(self, other: Union[complex, numbers.Number]) -> 'cirq.PauliString[TKey]':
def __mul__(self, other: numbers.Number) -> 'cirq.PauliString[TKey]':

Copy link
Collaborator Author

@daxfohl daxfohl Feb 7, 2025

Choose a reason for hiding this comment

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

Here also I went the opposite direction. numbers types are supposed to be for instanceof, but don't work well with mypy. SupportsComplex is the proper way to do it with mypy, but SupportsComplex comes with overhead. It only means it has __complex__ on it, which means you've got to call complex(other) before you can safely use it; code and time overhead. The only thing it improves is that mypy won't complain when using np.int64 or other tangential types, though they still work at run time regardless. So, just leaving this as complex, to be consistent with all our other complex parameter types.

@pavoljuhas
Copy link
Collaborator

A follow-up item could be to replace val.real etc. with np.real(val) in those cases, which would allow standardizing on isinstance(numbers.Complex) everywhere.

We should keep in mind here that attribute access is much faster than np.real -

In [3]: %timeit c.real
39.5 ns ± 1.52 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
In [4]: %timeit np.real(c)
231 ns ± 16.2 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

@daxfohl daxfohl added this pull request to the merge queue Feb 7, 2025
Merged via the queue into quantumlib:main with commit 72e1d20 Feb 7, 2025
38 checks passed
@daxfohl daxfohl deleted the number branch February 7, 2025 06:36
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 20, 2025
* Clean up redundant complex type unions

* format

* fix numpy ufunc, one mypy err

* Additional cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace (int, float, complex) and permutations with numbers.Number
3 participants