Skip to content

Fixes #7384 : Made DecompositionContext non-optional in _decompose_with_context_ me… #7385

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Soham-47
Copy link

@Soham-47 Soham-47 commented May 29, 2025

Fixes #7384 (comment)

  • Updated type annotations in SupportsDecompose and SupportsDecomposeWithQubits protocols
  • Made context parameter non-optional in all decompose_with_context implementations
  • Kept context optional in top-level decompose functions (decompose, decompose_once, decompose_once_with_qubits)
  • This change reflects that decompose_with_context is never called without a valid DecompositionContext

…thods

- Update type annotations in SupportsDecompose and SupportsDecomposeWithQubits protocols
- Make context parameter non-optional in all _decompose_with_context_ implementations
- Keep context optional in top-level decompose functions (decompose, decompose_once, decompose_once_with_qubits)
- This change reflects that _decompose_with_context_ is never called without a valid DecompositionContext
@Soham-47 Soham-47 requested review from vtomole and a team as code owners May 29, 2025 13:28
@Soham-47 Soham-47 requested a review from maffoo May 29, 2025 13:28
@github-actions github-actions bot added the size: S 10< lines changed <50 label May 29, 2025
@Soham-47 Soham-47 changed the title Fixes #7384: Made DecompositionContext non-optional in _decompose_with_context_ me… Fixes #7384 : Made DecompositionContext non-optional in _decompose_with_context_ me… May 29, 2025
@@ -126,7 +126,7 @@ def _decompose_(self) -> DecomposeResult:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

There were a couple other places in this file too iirc, like some type definitions.

@@ -832,7 +832,7 @@ def _decompose_(self) -> cirq.OP_TREE:
return self._decompose_with_context_()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think _decompose_ and _decompose_with_context_ should never both be implemented. If they are, then the protocol algorithm just runs twice and wastes CPU. So _decompose_ can be deleted in any file _decompose_with_context_ exists IIUC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also run check/pytest-changed-files locally to check the changes before pushing. check/mypy, check/format-incremental --apply, check/pylint-changed-files are also useful.

…nal functions

- Made context parameter required in _DecomposeArgs and _try_op_decomposer
- Kept context parameter optional in public API functions (decompose, decompose_once, etc.)
- Updated type annotations to reflect that _decompose_with_context_ is always called with a valid context
- Added proper null checks before using the context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DecompositionContext is never None
3 participants