Skip to content
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

Multi-level dynamic decompositions #6881

Merged
merged 113 commits into from
Feb 25, 2025
Merged

Conversation

PietropaoloFrisoni
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni commented Jan 24, 2025

Context: After implementing the first minimum working prototype in #6859, we consider the case where operators in the dynamic decompositions have another dynamic decomposition available. That is, another compute_plxpr_decomposition method.

Description of the Change: The interpreter can now handle both the max_expansion and gate_set arguments.

Benefits: More flexible and powerful implementation

Possible Drawbacks: The idea at the current stage is that the compute_plxpr_decomposition should always be preferred over the old static compute_decomposition method if it has been defined for an operator. However, there is one exception to this logic, which is a limitation in the current implementation due to code structure and time estimates.

In the dynamic decomposition of an operator with a compute_plxpr_decomposition method, if we encounter an operator without a compute_plxpr_decomposition method along the decomposition, then we switch to the current decomposition logic, which disables the program capture mechanism and decomposes recursively until we reach a max depth or a stopping condition, then re-actives program capture again at the end of the decomposition.

Because of this, if in the dynamic decomposition of an operator we encounter an operator without a compute_plxpr_decomposition and therefore we call the compute_decomposition method of that operator, if an operator with a compute_plxpr_decomposition is returned, we ignore that and proceed with the usual compute_decomposition method. See the test_nested_decomp_no_plxpr_decomp_max_exp test for a concrete example.

This is a convenient compromise that appears because:

  • we decided to entangle this logic with the standard decomposition pipeline
  • we have a compute_plxpr_decomposition defined for just a few operators, and a compute_decomposition is always present for such operators.
  • the current decomposition pipeline disables program capture globally and acts recursively, making the integration harder

In the future, this will not be an issue since we'll have a unique new way of performing decompositions.

Related GitHub Issues: None,

Related ShortCut Stories: [sc-83111]

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@PietropaoloFrisoni PietropaoloFrisoni changed the title First attempt (probably still not working) Multi-level dynamic decompositions Jan 24, 2025
Copy link

@josephleekl josephleekl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @PietropaoloFrisoni for the great addition! 🚀

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Thanks Pietro, I left one comment. Let me know what you think about it. I feel the way that I mentioned in the comment because if we can avoid changing the signature of methods defined in the base class, we should.

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Looks good. I left a non-blocking suggestion. If it's not addressed in this PR, could you create a shortcut story to track it?

Also one question about testing, but also non-blocking.

@PietropaoloFrisoni PietropaoloFrisoni added the merge-ready ✔️ All tests pass and the PR is ready to be merged. label Feb 25, 2025
@PietropaoloFrisoni PietropaoloFrisoni enabled auto-merge (squash) February 25, 2025 17:35
@PietropaoloFrisoni PietropaoloFrisoni merged commit 7c239b6 into master Feb 25, 2025
46 checks passed
@PietropaoloFrisoni PietropaoloFrisoni deleted the multi_level_decomp branch February 25, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-ready ✔️ All tests pass and the PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants