Skip to content

Fix mypy annotations in fermion_partitioning (#1282)#1349

Merged
mhucka merged 10 commits into
quantumlib:mainfrom
rosspeili:fix/issue-1282-mypy
Jun 10, 2026
Merged

Fix mypy annotations in fermion_partitioning (#1282)#1349
mhucka merged 10 commits into
quantumlib:mainfrom
rosspeili:fix/issue-1282-mypy

Conversation

@rosspeili

Copy link
Copy Markdown
Contributor

First slice for #1282: correct generator return types and local variable annotations in measurements/fermion_partitioning.py. No runtime or API behavior changes.

First slice for quantumlib#1282: correct generator return types and local variable annotations in measurements/fermion_partitioning.py. No runtime or API behavior changes.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds type annotations to several functions in src/openfermion/measurements/fermion_partitioning.py to improve type safety and code readability. Since there are no review comments, I have no feedback to provide.

@mhucka mhucka left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for doing this!

There is one change I would recommend. Although Any is valid, it is better to define so-called generic parameter types, along the following lines:

from typing import TypeVar

T = TypeVar('T')

def pair_within(labels: list[T]) -> Generator[tuple[T, ...], None, None]:
    """Generates pairings of labels that contain each pair at least once."""
    ...

Applying this across other matching functions (like pair_between and pair_within_simultaneously) allows static analyzers to check that the output tuples contain elements of the exact same type as the input.

Would you be able to try to make the appropriate changes to this PR?

Replace Any with generic T and a Pairing type alias that matches runtime: tuples of (T, T) pairs (plus occasional bare T leftovers). Internal iterator combiners stay on Any. No behavior changes.
@rosspeili

Copy link
Copy Markdown
Contributor Author

@mhucka reworked based on suggestion, mypy is clean on this file and all tests pass. Let me know how it looks, and thanks for the tip. <3

@mhucka mhucka left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not done with the review on the update, but here's a start for a minor detail.

Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
Comment thread src/openfermion/measurements/fermion_partitioning.py Outdated
rosspeili and others added 8 commits June 10, 2026 15:11
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
After Pairing[T] annotations, strict mypy flagged the internal None sentinel branch. Use cast and isinstance narrowing so yielded types stay Pairing[T] without changing runtime behavior.
@rosspeili

Copy link
Copy Markdown
Contributor Author

@mhucka applied all the Pairing[T] suggestions from above, but that made strict mypy complain about the None sentinel in pair_within, so I fixed that in a follow-up push, same behavior, all tests pass. Let me know how this looks or if I am missing something. Thanks again for the tips and direction <3

@mhucka mhucka left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Thank you for working on this so diligently!

@mhucka
mhucka enabled auto-merge June 10, 2026 21:24
@mhucka
mhucka added this pull request to the merge queue Jun 10, 2026
Merged via the queue into quantumlib:main with commit f3a4a5a Jun 10, 2026
35 of 37 checks passed
@rosspeili
rosspeili deleted the fix/issue-1282-mypy branch June 11, 2026 08:36
@rosspeili

Copy link
Copy Markdown
Contributor Author

@mhucka thanks for merging #1349

Happy to keep going with the same small-PR approach. Any preference for the next batch? I was thinking another pure-annotation file with a small error count, unless you'd rather prioritize a specific error family first.

@mhucka

mhucka commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

@rosspeili Thank you for your work on this. I can't think of a reason to prefer any particular order, so it's up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants