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

[unitaryhack] - Added circuit inverse #26

Merged
merged 9 commits into from
Jun 10, 2022

Conversation

golanor
Copy link
Contributor

@golanor golanor commented Jun 4, 2022

adding an inverse method to the Circuit class, see PR #10 - zapata-engineering/orquestra-core#10

Copy link
Contributor

@AthenaCaesura AthenaCaesura left a comment

Choose a reason for hiding this comment

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

This is great! Short and sweet and really functional thanks for your contribution!

However, it appears you're not passing some style and test checks. The easiest way to test this is if you have make installed, you can just run make muster from the main directory and it will tell you what changes need to be made.

src/orquestra/quantum/circuits/_circuit.py Outdated Show resolved Hide resolved
@golanor
Copy link
Contributor Author

golanor commented Jun 7, 2022

Thanks for the comments! Let me know if it is better this way.

By the way, it would be much easier to contribute if there was a requirements.txt file to create an environment on which to run the code.

@mstechly
Copy link
Contributor

mstechly commented Jun 7, 2022

We intend to specify all the requirements through setup.cfg, it also specified required Python versions.
How do you see having requirements.txt would be more helpful?

@golanor
Copy link
Contributor Author

golanor commented Jun 8, 2022

We intend to specify all the requirements through setup.cfg, it also specified required Python versions. How do you see having requirements.txt would be more helpful?

I was wrong - I'm just used to different package structures, where you don't require an editable install in order to work on the package. I think that with the src structure this way is better.

In any case, I added a test for having an AttributeError raised when the circuit cannot be inverted.

Copy link
Contributor

@AthenaCaesura AthenaCaesura left a comment

Choose a reason for hiding this comment

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

Nice improvement! Thanks for your work!

But it still looks like some of the tests are failing. I think this is because the EXAMPLE_OPERATIONS contain gates which depend on a variable. These are not as easily inverted (although it would be a nice feaure to see exactly which gate cannot be inverted 😉). There also seem to be some style issues related to a line being too long in _circuit_test.

Can you run the tests and style checks via make? It's not a problem if you don't. You'll just have to do the checks one by one.

@golanor
Copy link
Contributor Author

golanor commented Jun 9, 2022

Tests are passing and so do the style checks :)

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #26 (3695618) into main (6e1b5c5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #26   +/-   ##
=======================================
  Coverage   94.23%   94.24%           
=======================================
  Files          68       68           
  Lines        5519     5524    +5     
=======================================
+ Hits         5201     5206    +5     
  Misses        318      318           
Impacted Files Coverage Δ
src/orquestra/quantum/circuits/_circuit.py 96.15% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e1b5c5...3695618. Read the comment docs.

Copy link
Contributor

@mstechly mstechly left a comment

Choose a reason for hiding this comment

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

These changes should fix the style issues :)

tests/orquestra/quantum/circuits/_circuit_test.py Outdated Show resolved Hide resolved
src/orquestra/quantum/circuits/_circuit.py Outdated Show resolved Hide resolved
tests/orquestra/quantum/circuits/_circuit_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@AthenaCaesura AthenaCaesura left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution! Happy to say your PR has been accepted!
Would you like a Zapata T-shirt? If so, then just reply to this thread with an address and size and we'll get it to you.
Happy Hacking!

@AthenaCaesura AthenaCaesura merged commit 20974a5 into zapata-engineering:main Jun 10, 2022
@golanor
Copy link
Contributor Author

golanor commented Jun 14, 2022

I'd love a T-shirt - is it possible to give you my address in a non-public way? :)

@mstechly
Copy link
Contributor

Absolutely, just send me an e-mail at michal.stechly@zapatacomputing.com :)

qiyuan2016 pushed a commit that referenced this pull request Jul 18, 2022
5d4c008 Merge pull request #27 from zapatacomputing/ZQS-1112-add-flake8-pyproject
ba51d59 add comment for flake8p
d300fb8 feat: add flake8-pyproject dependency
d8f68df feat: update workflow templates (#26)

git-subtree-dir: subtrees/z_quantum_actions
git-subtree-split: 5d4c008d19478a3b297fa01bd713e724d0516658
qiyuan2016 pushed a commit that referenced this pull request Jul 19, 2022
5d4c008 Merge pull request #27 from zapatacomputing/ZQS-1112-add-flake8-pyproject
ba51d59 add comment for flake8p
d300fb8 feat: add flake8-pyproject dependency
d8f68df feat: update workflow templates (#26)

git-subtree-dir: subtrees/z_quantum_actions
git-subtree-split: 5d4c008d19478a3b297fa01bd713e724d0516658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants