-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Update T1 experiment #6487
Update T1 experiment #6487
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6487 +/- ##
==========================================
- Coverage 97.76% 97.76% -0.01%
==========================================
Files 1105 1105
Lines 94997 94999 +2
==========================================
+ Hits 92871 92872 +1
- Misses 2126 2127 +1 ☔ View full report in Codecov by Sentry. |
@NoureldinYosri do you know why the Windows pytest is failing? |
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.
@eliottrosenberg it's not clear to me what's happening with the windows test. I suggest adding some information to the assert
statement to try to understand what's going on.
@@ -121,7 +121,7 @@ def test_all_on_results(): | |||
data=pd.DataFrame( |
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.
can you change this line to
desired = ...
assert results == desired, f'{results.data=} {desired.data=}'
so that we get a more informative error message.
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.
I tried this, and it just outputs
> assert results == desired, f'{results.data=} {desired.data=}'
E AssertionError: results.data= delay_ns false_count true_count
E 0 100 10 0
E 1 215 10 0
E 2 464 10 0
E 3 1000 10 0 desired.data= delay_ns false_count true_count
E 0 100 10 0
E 1 215 10 0
E 2 464 10 0
E 3 1000 10 0
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.
I suspect it may have something to do with data types since I changed line 83 in t1_decay_experiment.py
to np.logspace(np.log10(min_delay_nanos), np.log10(max_delay_nanos), num_points, dtype=int)
.
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 like it's indeed a types issue specifically for the delay_ns
column
>>> results.data.dtypes
delay_ns float64
false_count int64
true_count int64
dtype: object
>>> desired.data.dtypes
delay_ns int64
false_count int64
true_count int64
dtype: object
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.
That's pretty strange. When I run it on linux, delay_ns
is int64
, and in line 83 of t1_decay_experiment.py
, I specifically ask for dtype=int
. What do you recommend? Should I manually convert delay_ns
to integers in the test? It seems like there is some deeper issue when running on windows.
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.
the result is computed by sampler.sample
which is probably changing the dtype to float internally. I suggest that you ensure that the desired
result is also float.
Why do you want to force delay_ns
to be int
?
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.
I changed it back to float
@@ -139,7 +139,7 @@ def test_all_off_results(): | |||
data=pd.DataFrame( |
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.
same as above
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.
LGTM%nits
@@ -77,7 +77,12 @@ def t1_decay( | |||
|
|||
var = sympy.Symbol('delay_ns') | |||
|
|||
sweep = study.Linspace(var, start=min_delay_nanos, stop=max_delay_nanos, length=num_points) | |||
if min_delay_nanos == 0: |
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.
use the original parameter to make it clear that this happens when no min_delay
is supplied
if min_delay is None
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.
I want to keep it as is in case the user specifies 0 min delay.
@@ -132,7 +137,8 @@ def exp_decay(x, t1): | |||
|
|||
# Fit to exponential decay to find the t1 constant | |||
try: | |||
popt, _ = optimize.curve_fit(exp_decay, xs, probs, p0=[t1_guess]) | |||
popt, _ = optimize.curve_fit(exp_decay, xs, probs, p0=[t1_guess, 1.0, 0.0]) |
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.
nit: you can assign directly
self.popt, _ = optimize.curve_fit(exp_decay, xs, probs, p0=[t1_guess, 1.0, 0.0])
* T1 experiment: add A and B constants to fit, make wait times log-spaced * lowercase variable names * update tests * update tests * update tests * update tests * update tests * update tests * update tests * informative assert statements in tests * use float wait time * typecheck * nits
* T1 experiment: add A and B constants to fit, make wait times log-spaced * lowercase variable names * update tests * update tests * update tests * update tests * update tests * update tests * update tests * informative assert statements in tests * use float wait time * typecheck * nits
Makes two changes to the$T_1$ experiment in Cirq: