Skip to content
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

T1 constant calculation #4290

Merged
merged 12 commits into from Jul 14, 2021
Merged

T1 constant calculation #4290

merged 12 commits into from Jul 14, 2021

Conversation

asmuzsoy
Copy link
Contributor

@asmuzsoy asmuzsoy commented Jul 7, 2021

Performs a curve fit to calculate t1 decay constant, includes option to plot curve fit

Addresses Issue #4264

@asmuzsoy asmuzsoy requested review from cduck, mrwojtek, vtomole and a team as code owners July 7, 2021 15:16
@asmuzsoy asmuzsoy requested a review from maffoo July 7, 2021 15:16
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jul 7, 2021
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

Good first pass! I left a few suggestions here and there for some minor tweaks in the code and then a few suggestions for cleaning up the tests.

I'm not sure if I was supposed to be a reviewer on this or not so I'm happy to hand off to others if need be.

cirq-core/cirq/experiments/t1_decay_experiment.py Outdated Show resolved Hide resolved
cirq-core/cirq/experiments/t1_decay_experiment.py Outdated Show resolved Hide resolved
cirq-core/cirq/experiments/t1_decay_experiment.py Outdated Show resolved Hide resolved
cirq-core/cirq/experiments/t1_decay_experiment.py Outdated Show resolved Hide resolved
cirq-core/cirq/experiments/t1_decay_experiment.py Outdated Show resolved Hide resolved
Comment on lines 216 to 233
bad_fit = cirq.experiments.T1DecayResult(
data=pd.DataFrame(
columns=['delay_ns', 'false_count', 'true_count'],
index=range(4),
data=[
[100.0, 10, 0],
[400.0, 10, 0],
[700.0, 10, 0],
[1000.0, 10, 0],
],
)
)

try:
bad_fit.plot(include_fit=True)
assert False
except RuntimeWarning as warning:
assert warning is not None
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be a good idea to split this error case off into a separate test method. You can also make use of

with pytest.raises(..., match='whatever the error string is'):
    ... something that breaks...

to verify an error gets raised in the test.

@@ -165,6 +167,72 @@ def test_all_off_results():
)


def test_constant():
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth looking into parameterizing this test with @pytest.mark.parameterize so that you can remove some of the code duplication between the 100 and 400 cases, and just turn the relevant values between these two results into parameters for the test. Here's a quick example: https://github.com/quantumlib/Cirq/blob/master/cirq-core/cirq/ops/common_gates_test.py#L60

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this suggestion! I've added this to the test, along with the explicitly exponential data from your other comment.

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

One minor comment then LGTM

good_fit.plot(include_fit=True)


def test_curve_fit_plot_error():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to indicate this is a warn and not an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for catching this!

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 14, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 14, 2021
@CirqBot CirqBot merged commit 68f7698 into quantumlib:master Jul 14, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 14, 2021
erichulburd pushed a commit to erichulburd/Cirq that referenced this pull request Jul 15, 2021
Performs a curve fit to calculate t1 decay constant, includes option to plot curve fit

Addresses Issue [quantumlib#4264](quantumlib#4264)
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Performs a curve fit to calculate t1 decay constant, includes option to plot curve fit

Addresses Issue [quantumlib#4264](quantumlib#4264)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants