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

Generalize concept of removing OpTypes #624

Merged
merged 10 commits into from
Jun 5, 2024
Merged

Conversation

lsschmid
Copy link
Collaborator

@lsschmid lsschmid commented Jun 4, 2024

Description

To not only remove Identities but also other Operations I wrote a function that generalizes that concept and allows one to remove arbitrary operation types.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@lsschmid lsschmid requested a review from burgholzer June 4, 2024 07:55
@lsschmid lsschmid self-assigned this Jun 4, 2024
@lsschmid
Copy link
Collaborator Author

lsschmid commented Jun 4, 2024

For some reason some other tests fail, not sure why.

Also, probably I don't need the functionality any more, so feel free to delete the PR

@burgholzer
Copy link
Member

For some reason some other tests fail, not sure why.

Also, probably I don't need the functionality any more, so feel free to delete the PR

Just from a Quick Look: The main reason for the tests failing is that the previous identity removal would also remove (multi-)controlled identities, while your version only removes single-qubit identities without controls.
That should be rather easy to fix though by adding a special case for the identity.

In general, I'd be in favour of getting this merged. It seems like a useful generalisation.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.6%. Comparing base (2782f37) to head (992da1a).
Report is 98 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #624   +/-   ##
=====================================
  Coverage   91.6%   91.6%           
=====================================
  Files        148     148           
  Lines      14731   14736    +5     
  Branches    2365    2365           
=====================================
+ Hits       13499   13504    +5     
  Misses      1232    1232           
Flag Coverage Δ
cpp 91.3% <100.0%> (+<0.1%) ⬆️
python 99.7% <ø> (ø)
Files with missing lines Coverage Δ
src/CircuitOptimizer.cpp 88.9% <100.0%> (+<0.1%) ⬆️

... and 1 file with indirect coverage changes

@lsschmid
Copy link
Collaborator Author

lsschmid commented Jun 4, 2024

Now it should work.
As said, I don't really need it anymore though.

Also: I wrote a comment on the compound operations. Please remove it if it looks fine.

It doesn't work for nested compound operations, but neither did the original implementation.

Signed-off-by: burgholzer <[email protected]>
@burgholzer burgholzer added enhancement New feature or request Core Anything related to the Core library and IR c++ Anything related to C++ code labels Jun 5, 2024
Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

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

Many thanks for the adaptation. I just pushed some small changes. Most notably, switched to unordered_set, which is a hash set and, thus, has fast membership testing (O(1) on average) compared to set (O(log n)). Not that it makes that much of a difference here, but I guess it's still desirable to use the most appropriate data types when easily possible.

@burgholzer burgholzer enabled auto-merge (squash) June 5, 2024 10:20
@burgholzer burgholzer merged commit 86d93b5 into main Jun 5, 2024
34 checks passed
@burgholzer burgholzer deleted the na-zx-hybrid-synthesis branch June 5, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code Core Anything related to the Core library and IR enhancement New feature or request
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants