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

Reorder Pauli terms before Trotterization #12925

Merged
merged 24 commits into from
Nov 7, 2024
Merged

Conversation

altaris
Copy link
Contributor

@altaris altaris commented Aug 8, 2024

Summary

Reordering the terms of a Pauli operator can reduce the depth of the evolution circuit if using the Lie-Trotter or Suzuki-Trotter methods. This PR adds the option to do just that.

Details and comments

This PR adds

  • the method synthesis.evolution.product_formula.reorder_paulis that reorders Pauli terms based on a greedy graph coloring heuristic;
  • the reorder option in the ProductFormula, LieTrotter and SuzukiTrotter constructors to allow for reordering before synthesis.

altaris added 2 commits August 8, 2024 17:24
Reordering the terms of a Pauli operator can reduce the depth of the evolution
circuit if using the Lie-Trotter or Suzuki-Trotter methods. This commit adds a
method `synthesis.evolution.product_formula.reorder_paulis` that does just that,
based on a greedy graph coloring heuristic. This commit also adds the `reorder` option in
the `ProductFormula`, `LieTrotter` and `SuzukiTrotter` constructors to allow for
reordering before synthesis.
@altaris altaris requested review from alexanderivrii, ShellyGarion and a team as code owners August 8, 2024 08:28
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Aug 8, 2024
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2024

CLA assistant check
All committers have signed the CLA.

@ShellyGarion ShellyGarion requested a review from Cryoris August 8, 2024 10:10
@coveralls
Copy link

coveralls commented Aug 8, 2024

Pull Request Test Coverage Report for Build 11729926405

Details

  • 45 of 47 (95.74%) changed or added relevant lines in 6 files are covered.
  • 9 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.004%) to 88.927%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/evolution/product_formula.py 25 26 96.15%
qiskit/synthesis/evolution/qdrift.py 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/qasm2/src/lex.rs 2 92.23%
crates/qasm2/src/parse.rs 6 97.62%
Totals Coverage Status
Change from base Build 11728606692: 0.004%
Covered Lines: 79062
Relevant Lines: 88907

💛 - Coveralls

@Cryoris
Copy link
Contributor

Cryoris commented Sep 2, 2024

@altaris are you still pursuing this or do would you like us to help? 🙂

@altaris
Copy link
Contributor Author

altaris commented Sep 2, 2024 via email

Before that, if reorder_paulis was given a single-term SparsePauliOp object, it
would return it encapsulated in a list. Now, it returns it as is.
@mtreinish mtreinish modified the milestones: 1.3 beta, 1.3.0 Sep 9, 2024
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Overall the PR looks very nice! I left some comments below, in particular about the handling of lists of operators. Could you also add a release note describing the new argument? 🙂

qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Just adding my 2 cents here

qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
If `reorder_paulis` is given a list of operators, then they are reordered and returned individually. Before this co
mmit, they used to be added all together and then reordered, which always resulted in a single operator.
Copy link
Member

@ShellyGarion ShellyGarion 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 for the nice contribution to Qiskit. Some release notes are needed.

Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks @Cryoris and the original contributor @altaris! I think allowing Pauli reordering is a useful option for the PauliEvolutionGate synthesis algorithms.

I have left a few minor comments and questions inline.

One more question: do you think that for Rustiq-based synthesis algorithm (and the plugin already accepts "preserve_order"), it might be beneficial to rearrange Paulis before passing the pauli network to Rustiq?

qiskit/circuit/library/pauli_evolution.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/lie_trotter.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/product_formula.py Outdated Show resolved Hide resolved
qiskit/synthesis/evolution/suzuki_trotter.py Outdated Show resolved Hide resolved
@Cryoris
Copy link
Contributor

Cryoris commented Nov 7, 2024

One more question: do you think that for Rustiq-based synthesis algorithm (and the plugin already accepts "preserve_order"), it might be beneficial to rearrange Paulis before passing the pauli network to Rustiq?

That is already possible to do with this PR, since the re-ordering happens before expand. So we can expose this in the Rustiq plugin, just like in the default plugin. So we would re-order with the graph coloring and then allow Rustiq to also re-order? Or are they redundant together?

@alexanderivrii
Copy link
Contributor

That is already possible to do with this PR, since the re-ordering happens before expand. So we can expose this in the Rustiq plugin, just like in the default plugin. So we would re-order with the graph coloring and then allow Rustiq to also re-order? Or are they redundant together?

I think it makes sense to expose this to Rustiq plugin as well. And I think it makes sense to let Rustiq do the additional reordering.

@Cryoris
Copy link
Contributor

Cryoris commented Nov 7, 2024

56e619c includes this option in the Rustiq plugin too 👍🏻

@@ -60,6 +60,7 @@ def __init__(
| None
) = None,
wrap: bool = False,
preserve_order: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

I thought the consensus here was to not reorder terms by default because of the potential performance overhead. Shouldn't the default be to True?

@@ -63,6 +69,7 @@ def __init__(
| None
) = None,
wrap: bool = False,
preserve_order: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

Same question here

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
preserve_order: bool = False,
preserve_order: bool = True,

qiskit/synthesis/evolution/qdrift.py Show resolved Hide resolved
Comment on lines 1538 to 1540
if "preserve_order" in options and isinstance(algo, ProductFormula):
algo.preserve_order = options["preserve_order"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this. A very small nitpick: we don't need another call to isinstance(also, ProductFormula) here, since we've just checked this on the line above.

@mtreinish
Copy link
Member

I think given the open questions and the proximity to 1.3.0 (ie this is the last PR being tracked) it's probably better to defer this to 2.0.0 rather than force it in. This feature is a nice to have, but I don't think it's absolutely necessary for 1.3.0

@raynelfss
Copy link
Contributor

Pushing to 2.0.0 after further discussion...

@raynelfss raynelfss modified the milestones: 1.3.0, 2.0.0 Nov 7, 2024
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks @Cryoris. This PR very nicely concludes the work that was done to improve the support for Pauli evolutions: representing them as gates, improving expansion methods, adding the plugin interface, new synthesis algorithms and new options, porting relevant code to Rust.

@raynelfss raynelfss modified the milestones: 2.0.0, 1.3.0 Nov 7, 2024
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this as is. I just have 2 small nits inline, but they can be fixed in a followup without issue and aren't worth blocking this over.

Comment on lines +83 to +85
preserve_order: If ``False``, allows reordering the terms of the operator to
potentially yield a shallower evolution circuit. Not relevant
when synthesizing operator with a single term.
Copy link
Member

Choose a reason for hiding this comment

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

I think from a docs perspective it would be good to explain the tradeoffs here, especially around runtime performance.

terms = sorted(paulis, key=_term_sort_key)
graph = rx.PyGraph()
graph.add_nodes_from(terms)
indexed_nodes = list(enumerate(graph.nodes()))
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't actually need to cast this is as a list, it should all work fine without it and save a copy.

@raynelfss raynelfss enabled auto-merge November 7, 2024 19:38
@mtreinish mtreinish disabled auto-merge November 7, 2024 19:57
@mtreinish mtreinish merged commit 4251d01 into Qiskit:main Nov 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.