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.

Sign up for GitHub

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

Jump to bottom

Add Fusion variations #1110

Merged
merged 10 commits into from
Mar 10, 2021
Merged

Add Fusion variations #1110

merged 10 commits into from
Mar 10, 2021

Conversation

hhorii
Copy link
Collaborator

@hhorii hhorii commented Jan 25, 2021 •
edited
Loading

Summary

Add fusion variations

Details and comments

Currently cost-based-fusion is an only method of fusion. This PR adds 1- and 2-Qubit fusion, which greedily fuse operations with consideration of commutativity in gate operations. To add them, this PR introduces AER::Transpile::Fuser, which is the base class of three fusion methods.

  • Add N-Qubit fusion
  • Add tests

@hhorii hhorii force-pushed the fusion_variations branch 7 times, most recently from dbb6c26 to e60275c Compare January 26, 2021 11:55
@hhorii hhorii changed the title [WIP] Add Fusion variations Add Fusion variations Jan 26, 2021
@chriseclectic chriseclectic added this to the Aer 0.8 milestone Feb 16, 2021
@hhorii
Copy link
Collaborator Author

hhorii commented Feb 17, 2021

@vvilpas Tests were failed because of a issue in recent MPI optimization. @doichanj is sending a PR to fix this issue.

@doichanj doichanj mentioned this pull request Feb 19, 2021
Merged
vvilpas
vvilpas reviewed Feb 19, 2021
View reviewed changes
Copy link
Contributor

@vvilpas vvilpas left a comment

Choose a reason for hiding this comment

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

I leave a few comments, while we wait for Doi's PR. Looks like they are many, but a large fraction of them are just to add forgotten override keyword.

src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp Show resolved Hide resolved
@doichanj doichanj mentioned this pull request Feb 24, 2021
Merged
@hhorii
Copy link
Collaborator Author

hhorii commented Mar 3, 2021

#1149 will resolve fails in test.

@chriseclectic
Copy link
Member

I think this needs rebasing to remove #1149 now that the bug fixes have been merged into master. Otherwise we need to wait for 1149 to be merged before this can be merged

@chriseclectic chriseclectic reopened this Mar 5, 2021
chriseclectic
chriseclectic reviewed Mar 5, 2021
View reviewed changes
Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

I have a couple questions about diagonal param in fusion. The main one is if we are fusing a set of diagonal matrices, we should not use the unitary simulator since that is inefficient, but directly update the diagonal vector instead.

src/framework/operations.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp
virtual bool support_diagonal() const = 0;

// Aggregate a subcircuit of operations into a single operation
virtual op_t generate_operation(std::vector<op_t>& fusioned_ops, bool diagonal = false) const {
Copy link
Member

Choose a reason for hiding this comment

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

Does diagonal mean all ops in fusioned_ops are diagonal, or that the output is diagonal even if the ops are not?
If its the first we should have a specialized simulation method that doesn't require building the full square matrix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the second because CNOT-diagonal-CNOT patttern is detected.

I implemented more efficient version to generate a diagonal matrix but performance improvement was very limited for generation of small diagonal matrix.

src/transpile/fusion.hpp
for (size_t i = 0; i < fusioned_op.qubits.size(); i++)
fusioned_op.qubits[i] = remapped2orig[fusioned_op.qubits[i]];

if (diagonal) {
Copy link
Member

Choose a reason for hiding this comment

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

To continue from above comment. If all ops are diagonal, we should have generate_operation_internal take the diagonal arg and return the diagonal operation if the method supports it.

src/transpile/fusion.hpp Outdated Show resolved Hide resolved
src/transpile/fusion.hpp
public:
virtual std::string name() override { return "superop"; };

virtual bool support_diagonal() const override { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

We could add a diagonal_superop instruction for the density matrix / superop simulators

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true. I disabled for them to test only statevector first.

src/transpile/fusion.hpp Outdated Show resolved Hide resolved
@vvilpas vvilpas requested a review from mtreinish as a code owner March 10, 2021 10:18
@vvilpas vvilpas merged commit 1994b35 into Qiskit:master Mar 10, 2021
@chriseclectic chriseclectic added the Changelog: New Feature Include in the Added section of the changelog label Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseclectic chriseclectic chriseclectic left review comments

@vvilpas vvilpas vvilpas approved these changes

@atilag atilag Awaiting requested review from atilag

@mtreinish mtreinish Awaiting requested review from mtreinish

Assignees
No one assigned
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Milestone
Aer 0.8
Development

Successfully merging this pull request may close these issues.
3 participants
@hhorii @chriseclectic @vvilpas