Skip to content

Fix remaining mypy errors for issue #1282 (PR1)#1387

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

Fix remaining mypy errors for issue #1282 (PR1)#1387
mhucka merged 5 commits into
quantumlib:mainfrom
rosspeili:fix/issue-1282-mypy-remaining

Conversation

@rosspeili

Copy link
Copy Markdown
Contributor

Clear strict mypy with full follow-imports across 20 files: hamiltonians, transforms, resource estimates, and circuit generators. Generator functions now return Iterator[cirq.OP_TREE], add ase.* to mypy optional-dep overrides.

Clear strict mypy with full follow-imports across 20 files: hamiltonians, transforms, resource estimates, and circuit generators. Generator functions now return Iterator[cirq.OP_TREE]; add ase.* to mypy optional-dep overrides.

@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 focuses on improving type safety and static analysis compliance across the codebase. Key changes include updating return types from cirq.OP_TREE to Iterator[cirq.OP_TREE], adding type annotations, introducing explicit type casts, and resolving various type-checking issues. The review feedback suggests modernizing type hints in isdf.py by using the standard built-in tuple instead of Tuple from the typing module, as the project targets Python 3.9 or newer.

Comment thread src/openfermion/resource_estimates/pbc/thc/factorizations/isdf.py Outdated
Comment thread src/openfermion/resource_estimates/pbc/thc/factorizations/isdf.py Outdated
Align isdf, binary_codes, hamiltonian, sparse_integrals, and hartree_fock with modern PEP 585 style already used elsewhere in the batch.

@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.

This looks great. My one comment is about the use of assert statements for what appears to be run-time value checking.

Comment thread src/openfermion/hamiltonians/jellium.py Outdated
Comment thread src/openfermion/hamiltonians/jellium.py Outdated
Address PR review: use if/raise instead of assert for period_cutoff narrowing so checks cannot be disabled with -O.
@rosspeili

rosspeili commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Hey @mhucka, sorry about that, that was my bad, asserts were only for mypy, not real runtime checks. I first swapped for raise ValueError(...), but that wasn’t great either >.< those branches were unreachable because we already default period_cutoff earlier, and ci flagged that.

Latest commit pulls default into a small _non_periodic_period_cutoff() helper used in both spots. Same behavior, no assert, no dead raise paths, and mypy is happy.

Thanks for the review, and direction. Let me know if I am missing anything. <3

Replace unreachable ValueError branches with _non_periodic_period_cutoff so mypy narrowing and coverage both pass without assert or dead raise paths.

@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. Thanks!

@mhucka
mhucka added this pull request to the merge queue Jun 29, 2026
@mhucka
mhucka removed this pull request from the merge queue due to a manual request Jun 29, 2026
@mhucka
mhucka added this pull request to the merge queue Jun 29, 2026
Merged via the queue into quantumlib:main with commit d1325e3 Jun 29, 2026
23 checks passed
@rosspeili
rosspeili deleted the fix/issue-1282-mypy-remaining branch June 30, 2026 08:25
@mhucka mhucka mentioned this pull request Jul 1, 2026
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