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] Adding a power wrapper for Gates. #27

Merged

Conversation

upsideon
Copy link
Contributor

Description

Fixes orquestra-core/#14 by adding a power wrapper for Gate instances. The wrapper allows arbitrary powers of existing gates to be used without having to explicitly define them in code. This adds additional flexibity for those using Orquestra.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #27 (04af026) into main (20974a5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   94.24%   94.29%   +0.04%     
==========================================
  Files          68       68              
  Lines        5524     5570      +46     
==========================================
+ Hits         5206     5252      +46     
  Misses        318      318              
Impacted Files Coverage Δ
src/orquestra/quantum/circuits/_gates.py 94.39% <100.00%> (+1.16%) ⬆️
src/orquestra/quantum/circuits/_serde.py 98.21% <100.00%> (+0.10%) ⬆️

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 20974a5...04af026. Read the comment docs.

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 PR! looking forward to merging!

It looks like your version of Black might be different than the one we are using. The spacing. between the "**" might cause issues later when we try to merge. But I think it shoudl be fine.

class TestDagger:
def test_power_of_dagger(self, gate):
if len(gate.free_symbols) == 0:
gate.dagger.power(0.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but why is this its own class? We have several tests of dagger above. Maybe we should either put this test up there or take all the dagger tests and put them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put that test in it's own class as I didn't see a test class dedicated to testing Dagger and wasn't aware that TestMatrixFactoryGate was playing that role. I'm my opinion I think that it would be best to have tests specific to Dagger and MatrixFactoryGate in their own classes respectively. That would allow us to easily find tests specific to a class and also have more certainty that a hypothetical test failure is related to the implementation Dagger rather than that of MatrixFactoryGate.

For the time being, I've removed the TestDagger class and moved the new Dagger test into TestMatrixFactoryGate with the others. If you agree with refactoring those tests in the way I proposed, I think it would be good to apply it in a separate pull request to keep the scope of this one well defined.

src/orquestra/quantum/circuits/_serde.py Show resolved Hide resolved
@upsideon
Copy link
Contributor Author

Thanks for the review @AthenaCaesura! With regards to the formatting, which version of Black are you using? Maybe there are is some custom Black configuration that isn't included in pyproject.toml, if the version turns out not to be the discrepancy?

@mstechly
Copy link
Contributor

mstechly commented Jun 10, 2022

@upsideon you can find versions of the dev packages we're using here – we keep it there to make it easier to have it consistent across multiple repos :)
So for black it's black~=22.3, I think you might be using 21, as I think they changed the spacing around ** in between these versions.

@upsideon
Copy link
Contributor Author

Thanks, @mstechly! I've updated to Black 22.3 and applied the resulting style updates.

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 PR! It will make a really nice addition to our codebase!
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.
I will discuss with the person who wrote the tests for dagger about moving them into their own class. 🙂
Happy Hacking!

@AthenaCaesura AthenaCaesura merged commit ca7fbe9 into zapata-engineering:main Jun 11, 2022
@mstechly
Copy link
Contributor

@upsideon maybe instead of providing your address here, just send me an e-mail at michal.stechly@zapatacomputing.com ;)

@upsideon upsideon deleted the add-power-wrapper-for-gates branch June 11, 2022 20:00
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement power wrapper for Gates
4 participants