q-optimize / c3

Toolset for control, calibration and characterization of physical systems
https://c3-toolset.readthedocs.io/
Apache License 2.0
69 stars 35 forks source link

Fix xy angle sign #232

Closed YonatanGideoni closed 2 years ago

YonatanGideoni commented 2 years ago

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

YonatanGideoni commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

codecov[bot] commented 2 years ago

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.