Skip to content

Fix xy angle sign #232

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

Merged
merged 8 commits into from
Aug 12, 2022
Merged

Conversation

YonatanGideoni
Copy link
Contributor

What

This PR fixes the wrong sign that was used for the xy_angle in the phase in the Instruction class.

Why

Makes rotations on the bloch sphere be in the correct direction. Closes #231

How

Removed minus sign.

Remarks

None

Checklist

Please include and complete the following checklist. Your Pull Request is (in most cases) not ready for review until the following have been completed. You can create a draft PR while you are still completing the checklist. Check the Contribution Guidelines for more details. You can mark an item as complete with the - [x] prefix

  • Tests - Added unit tests for new code, regression tests for bugs and updated the integration tests if required
  • Formatting & Linting - black and flake8 have been used to ensure styling guidelines are met
  • Type Annotations - All new code has been type annotated in the function signatures using type hints
  • Docstrings - Docstrings have been provided for functions in the numpydoc style
  • Documentation - The tutorial style documentation has been updated to explain changes & new features
  • Notebooks - Example notebooks have been updated to incorporate changes and new features
  • Changelog - A short note on this PR has been added to the Upcoming Release section

@YonatanGideoni
Copy link
Contributor Author

The cases where the tests are failing seem to be because of fairly small differences in the resulting optimal parameters for various optimisations/calibrations (except for maybe test_flux_signal, where the differences are on the order of ~10%). What should I do to deal with this? Create new parameter files? @lazyoracle

@lazyoracle
Copy link
Member

The cases where the tests are failing seem to be because of fairly small differences in the resulting optimal parameters for various optimisations/calibrations (except for maybe test_flux_signal, where the differences are on the order of ~10%). What should I do to deal with this? Create new parameter files? @lazyoracle

In general, failing tests should not be modified to get them to pass. Do you possibly have some insight on why the optimal parameters changed? If however the change in the test optimal parameters is expected as a result of the code change in the PR, then feel free to just modify the parameters and move on. Make sure to note in your commit message the reasoning behind why the test was modified.

@nwittler
Copy link
Collaborator

With optimization results, it can be tricky when the convention changed. An alternative to what Anurag suggested would be to change the gate definitions in those tests from phase -> -phase to recreate the previous behavior. Then add a note that the convention here goes the "wrong" way.
If this does not work, something else is wrong.

Copy link
Collaborator

@nwittler nwittler left a comment

Choose a reason for hiding this comment

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

Looks good and can be approved when the discussed problems are resolved.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 11, 2022
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #232 (6cb0563) into dev (76bdf7b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev     #232   +/-   ##
=======================================
  Coverage   74.82%   74.82%           
=======================================
  Files          38       38           
  Lines        5534     5534           
=======================================
  Hits         4141     4141           
  Misses       1393     1393           
Impacted Files Coverage Δ
c3/signal/gates.py 95.02% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lazyoracle lazyoracle merged commit 01cd657 into q-optimize:dev Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong sign in xy_angle in phase in Instruction class
3 participants