Skip to content

Conversation

@astralcai
Copy link
Contributor

@astralcai astralcai commented Aug 28, 2025

Context:
The DecompositionGraph currently errors out completely if any of the operators is unsolved for, and as a result, with graph enabled, if a circuit contains a single operator/template that is not yet integrated with the new decomposition system, the entire thing fails, and the user is forced to turn off graph mode and use the old decomposition system. We want to change this behaviour, make it so that the DecompositionGraph raise a warning instead of an error if certain operators are unsolved for, and fall back to using op.decomposition for those operators.

Description of the Change:

  • Downgrade error to a warning when certain operators are unsolved in the graph.
  • Add sensible warning for when the graph failed to find a solution due to GlobalPhase.
  • When the new system is enabled, operators without a decomposition will be left undecomposed with a warning.

Benefits:

  • The user can use the new system for the rest of the circuit even if it contains templates that are not yet integrated with the new graph-based system.

Possible Drawbacks:

Related GitHub Issues:
[sc-98390]

@astralcai astralcai requested a review from isaacdevlugt August 28, 2025 17:22
@astralcai astralcai changed the title [WIP] Fall back to op.decompose if op is unsolved in decomposition graph Fall back to op.decompose if op is unsolved in decomposition graph Aug 28, 2025
@astralcai astralcai marked this pull request as ready for review August 28, 2025 17:50
@codecov
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.69%. Comparing base (8612788) to head (d8d78fb).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8156      +/-   ##
==========================================
- Coverage   99.69%   99.69%   -0.01%     
==========================================
  Files         548      548              
  Lines       56815    56805      -10     
==========================================
- Hits        56641    56631      -10     
  Misses        174      174              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@JerryChen97 JerryChen97 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this fallback logic! Would you like to add a test over a dummy operator with no matrix to see it gets channelled back to old decomposition and emits DeviceError as well? Something like:

class NoMatNoDecompOp(qml.operation.Operation):
    """Dummy operation for checking check_validity throws error when
    expected."""

    # pylint: disable=arguments-renamed, invalid-overridden-method
    @property
    def has_matrix(self):
        return False

@astralcai
Copy link
Contributor Author

Thanks for implementing this fallback logic! Would you like to add a test over a dummy operator with no matrix to see it gets channelled back to old decomposition and emits DeviceError as well? Something like:

class NoMatNoDecompOp(qml.operation.Operation):
    """Dummy operation for checking check_validity throws error when
    expected."""

    # pylint: disable=arguments-renamed, invalid-overridden-method
    @property
    def has_matrix(self):
        return False

That would be irrelevant to this PR. The behaviour you're talking about is exclusive to preprocess.decompose.

@JerryChen97
Copy link
Contributor

Thanks for implementing this fallback logic! Would you like to add a test over a dummy operator with no matrix to see it gets channelled back to old decomposition and emits DeviceError as well? Something like:

class NoMatNoDecompOp(qml.operation.Operation):
    """Dummy operation for checking check_validity throws error when
    expected."""

    # pylint: disable=arguments-renamed, invalid-overridden-method
    @property
    def has_matrix(self):
        return False

That would be irrelevant to this PR. The behaviour you're talking about is exclusive to preprocess.decompose.

This PR does not involve any Operator with has_matrix=False?

@astralcai
Copy link
Contributor Author

Thanks for implementing this fallback logic! Would you like to add a test over a dummy operator with no matrix to see it gets channelled back to old decomposition and emits DeviceError as well? Something like:

class NoMatNoDecompOp(qml.operation.Operation):
    """Dummy operation for checking check_validity throws error when
    expected."""

    # pylint: disable=arguments-renamed, invalid-overridden-method
    @property
    def has_matrix(self):
        return False

That would be irrelevant to this PR. The behaviour you're talking about is exclusive to preprocess.decompose.

This PR does not involve any Operator with has_matrix=False?

Whether or not an operator has matrix is completely irrelevant here. The has_matrix check is part of DefaultQubit's stopping condition. This PR is about making it so that when the new graph-based system is not able to find a decomposition for an operator, use op.decomposition() as a fall back. This has nothing to do with the stopping condition whatsoever.

@astralcai astralcai enabled auto-merge September 4, 2025 16:08
@astralcai astralcai added this pull request to the merge queue Sep 4, 2025
@astralcai astralcai removed this pull request from the merge queue due to a manual request Sep 4, 2025
@astralcai astralcai added this pull request to the merge queue Sep 4, 2025
Merged via the queue into master with commit b51f590 Sep 4, 2025
52 checks passed
@astralcai astralcai deleted the decomp-fallback branch September 4, 2025 18:06
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.

7 participants