Closed lneuhaus closed 3 years ago
Merging #605 (aa8a9c1) into master (afde299) will increase coverage by
0.04%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #605 +/- ##
==========================================
+ Coverage 98.50% 98.55% +0.04%
==========================================
Files 77 77
Lines 8859 8857 -2
==========================================
+ Hits 8727 8729 +2
+ Misses 132 128 -4
Impacted Files | Coverage Δ | |
---|---|---|
strawberryfields/tdm/tdmprogram.py | 97.84% <100.00%> (+2.10%) |
:arrow_up: |
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 afde299...aa8a9c1. Read the comment docs.
@thisac I added a few tests to make sure the validation works as one would expect. I checked that these tests fail on the current master branch, so am confident they cover the bug that we're fixing here.
The failed codecov-check seems to be unrelated to this PR - I will assume someone will fix this in another branch and probably just wait for that solution?
Context: We recently changed the allowed gate parameters for
TD2
. TheRgate
phase can now be both positive and negative. With this change, requested programs did not pass the validation step in the program validation any more when negative phases were set. The reason was that when the code was supposed to validate the phases of theBSgate
(where only positive values are allowed), it used the values set for theRgate
phases, which were occasionally negative.Description of the Change: The proposed solution now uses the correct values in the above described validation step for each parameter.
Benefits: Fixed a bug. Code behavior should be closer to what one expects now.
Possible Drawbacks: I did not fully understand the logic. In particular, it seems to me that the current way of updating a counter for the index that is used to grab the relevant parameter values is highly error-prone. I could imagine there are edge cases where this still leads to unexpected behavior. A more thorough logic cleanup would maybe be a better solution, but take more time. I also did not dig too far into why the tests did not uncover this problem. I'd be happy to add a new test case in case it's worth it and we don't want to do the full cleanup now.
Related GitHub Issues: None