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

Remove qextract_p and qinst_p from forced-order primitives #469

Merged
merged 7 commits into from Jan 22, 2024

Conversation

Mandrenkov
Copy link
Contributor

@Mandrenkov Mandrenkov commented Jan 19, 2024

Context:

Generating the JAX jaxpr from a PennyLane QNode composed of many (unrolled) gates can be quite slow. This is partially due to the topological sorting of jaxpr equations which form a dependency graph with a near-quadratic number of edges. Most of these edges are a result of forced-order constraints that are added using:

for i, q in fixedorder:
   # [This nested loop introduces quadratic complexity.]
    for b in boxes[i + 1 :]:
        b.parents.append(q)

Currently, the set of forced-order primitives is {qdevice_p, qextract_p, qinst_p}; however, the data dependencies of qextract_p and qinst_p should already be captured by the relationships of input and output variables in the jaxpr.

Description of the Change:

  • Removed qextract_p and qinst_p from the list of forced-order primitives.

Benefits:

  • Obtaining the jaxpr of a quantum circuit with many gates is quadratically faster (up to a factor).

Possible Drawbacks:

  • Constraints between qextract_p and qinst_p primitives must (continue to) be encoded via input-output variable dependencies.

Related GitHub Issues:

None.

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Great first contribution to Catalyst 💯

frontend/catalyst/jax_tracer.py Show resolved Hide resolved
doc/changelog.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cc62f65) 99.56% compared to head (94065f1) 99.56%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #469   +/-   ##
=======================================
  Coverage   99.56%   99.56%           
=======================================
  Files          43       43           
  Lines        7666     7666           
  Branches      514      514           
=======================================
  Hits         7633     7633           
  Misses         17       17           
  Partials       16       16           

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

doc/changelog.md Outdated Show resolved Hide resolved
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

This change was driven by a performance issue, correct? What was the impact on performance before and after this change? Just saw the ticket comment. 😎

Thanks!

@Mandrenkov Mandrenkov merged commit 094d867 into PennyLaneAI:main Jan 22, 2024
21 checks passed
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.

None yet

4 participants