-
Notifications
You must be signed in to change notification settings - Fork 6
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
In VZ signal experiment sweep phase as absolute, instead of relative to match the non-signal experiment #1017
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1017 +/- ##
==========================================
+ Coverage 97.46% 97.47% +0.01%
==========================================
Files 123 123
Lines 9722 9722
==========================================
+ Hits 9476 9477 +1
+ Misses 246 245 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Both should be the same. Thanks!
Actually, could we do it the other way around? Change the the other routine to match with the signal one? The point of this is that after setting the virtual phase, you want to see both signals starting at the same point. |
I prefer the current implementation (just |
Indeed, to be more clear we can draw a vertical line where the virtual phase is supposed to be. |
Okay, the vertical line might add clarity. I would also encourage to keep the virtual phases modulo 2\pi then, as otherwise they might not appear on the graph. |
Somehow visualizing the calculated angles (be it a vertical line, or other means) sounds good. Using angles modulo 2*pi if it is not the case already - completely agree. Since these are irrelevant to the problem that this PR is addressing, let's create separate issue(s) for these requests and address in them in a separate PR. |
resolves #1004