-
Notifications
You must be signed in to change notification settings - Fork 127
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
T phi experiment #355
T phi experiment #355
Conversation
self._delays_t1 = delays_t1 | ||
self._delays_t2 = delays_t2 | ||
self._unit = unit | ||
self._osc_freq = osc_freq |
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 options
expT2 = T2Ramsey(qubit, self._delays_t2, self._unit, self._osc_freq) | ||
exps = [] | ||
exps.append(expT1) | ||
exps.append(expT2) |
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.
exps = [expT1, expT2]
exps.append(expT2) | ||
|
||
# Run batch experiments | ||
batch_exp = super().__init__(exps) |
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 variable batch_exp
is not used, just call super().__init__
without storing the result in a variable
# Run batch experiments | ||
batch_exp = super().__init__(exps) | ||
|
||
def run(self, backend, experiment_data, **run_options): |
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.
Don't override run
@@ -108,7 +108,7 @@ def run(self, run_input, **options): | |||
prob1[qubit] = 1 - prob1[qubit] | |||
elif op.name == "delay": | |||
delay = op.params[0] | |||
prob1[qubit] = prob1[qubit] * np.exp(-delay / self._t1[qubit]) | |||
prob1[qubit] = prob1[qubit] * np.exp(-delay / self._t1) |
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 [qubit]
term should remain there. Different qubits can have different T1s and the user has to provide an array of T1s when initializing the backend.
@yaelbh , thanks for the review but it is very premature. It is still WIP. |
…ent and to CompositeExperiment. Fix in return value of CompositeAnalysis._run_analysis.
# Wait for all component analysis to finish before returning | ||
# the parent experiment analysis results | ||
for comp_id in component_ids: | ||
experiment_data.child_data(comp_id).block_for_results() | ||
|
||
return [], [] | ||
return experiment_data, [] |
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.
?
… results. Added tests for Tphi
…ns instead of plain parameters for setting delays_t1 and delays_t2 in Tphi.
backend: Optional, the backend on which to run the experiment | ||
""" | ||
# Set experiment options | ||
options = self._default_experiment_options() |
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.
Is there a reason not to do it in a way that's different from the other experiments?
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.
If instead I do self.experiment_options.delays_t1 = delays_t1
, I get the error AttributeError: 'Tphi' object has no attribute '_experiment_options'
.
Same error if I do self.set_experiment_options(delays_t1=delays_t1, delays_t2=delays_t2)
.
I believe the reason is that I do the super.init()
later, so the base class members are not recognized yet.
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.
Ok, so you can do it later also here. I'm just not sure if what you're doing is equivalent, so I don't want to play with it.
features: | ||
- | | ||
New experiment Tphi. This is a composite experiment with sub-experiments | ||
T1 and T2Ramsey.New classes: |
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.
T1 and T2Ramsey.New classes: | |
T1 and T2Ramsey. New classes: |
I suggest to explain what's the new experiment about. The release notes are exposed to users and have to be informative for them.
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.
Added a short description of the experiment.
options.delays_t1 = delays_t1 | ||
options.delays_t2 = delays_t2 | ||
|
||
self.exps = [] |
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.
Maybe exps = [T1(...), T2Ramsey(...)]
?
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.
You were right about the bug - very nice of you to think of it! See my last commit to address that case.
You may have a bug: what happens if the user initializes the
You can write a test to check (that will remain also after you fix). I think the T1 experiment will continue to run with the old delays. You probably have to override
|
…for the sub-experiments, without storing the options for the main object. Removed _default_experiment_options, that is no longer needed
…et_experiment_options after run
… following changes in PR 633
Hi @yaelbh , I think this is ready for a (hopefully) final review. |
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.
One more small fix
self.component_experiment(0).set_experiment_options(delays=fields["delays_t1"]) | ||
elif key == "delays_t2": | ||
self.component_experiment(1).set_experiment_options(delays=fields["delays_t2"]) | ||
else: |
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.
osc_freq
is missing
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.
Good catch! Added it + a test.
# propagate options to the sub-experiments. | ||
for key in fields: | ||
if key == "delays_t1": | ||
self.component_experiment(0).set_experiment_options(delays=fields["delays_t1"]) |
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.
There's a subtle point here, and I think what you're doing is the most appropriate.
The experiment options are exposed to the user by the property BaseExperiment.experiment_options
. I understand that you wish to mask the internal implementation of a batch experiment consisting of sub-experiments. But a user who'll try tphi_exp.experiment_options.delays_t1
will get an error, because Tphi doesn't really have an option named delays_t1
.
You can try to solve this by maintaining options delays_t1
and delays_t2
in Tphi, in addition to the delay options of the sub-experiments. But then what will you do if the user does tphi_exp.experiment_options.delays_t1 = ...
. You'll face again the issue of having to sync with the delays of the sub-experiment.
You'll keep facing similar issues if you try to override experiment_options
. Possibly the only way to obtain full masking is by overriding Options
, which is exaggerated. Hence the bottom line (which I already wrote in the upper line) that the current code is the most reasonable one, in my opinion.
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.
If this were C++, I could have one of the delays_t2 (in Tphi and in T2Ramsey) fields point to the other. But as far as I know, there is no pointer concept in Python, right?
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.
Thanks @merav-aharoni, LGTM.
One last thing: check if experiment is done, as @nkanazawa1989 suggested yesterday in slack. Aftet that this can be merged, no need for further review (I'm approving now).
* Beginning implementation of T_phi * Added init files * Built circuits for T_phi * Defined class T_phi * Added run method to class Tphi * First stage of Tphi - up to run * Added test for Tphi. Added backend for testing, based on T1 and T2Ramsey backends. * black * Fixed return FakeJob from TphiBackend.run * Get sub-experiment analysis results from expdata.child_data * Added _run_analysis for TphiAnalysis. Added parameter to BatchExperiment and to CompositeExperiment. Fix in return value of CompositeAnalysis._run_analysis. * Cleaned up return value for Tphi analysis results. Computed the final results. Added tests for Tphi * Removed usage of 'units' in Tphi. Added usage of set_experiment_options instead of plain parameters for setting delays_t1 and delays_t2 in Tphi. * Removed unnecessary variable 'unit' and unnecessary imports. * Added missing 'TphiAnalysis in __init__ * Added documentation for Tphi * Added 'analysis' parameter to BatchExperiment and to CompositeExperiment * Added title and section to tphi tutorial * Fixed Tphi computation. Wrote tutorial. Added computation with uncertainties. * Fixed title of tutorial * Removed unnecessary prints and empty lines * Change of parameter merged from main * Added references for Tphi * Updated copyright to 2022. Added _set_backend to Tphi * Bug fix: parameter 'backend' was missing in calls to T1 and T2Ramsey * Cleaned up Tphi tutorial * Removed dataProcessor from default_options because it was causing a warning when loading from a device * Added release notes for tphi * Syntax changes resulting from code review * Update qiskit_experiments/library/characterization/tphi.py Co-authored-by: Yael Ben-Haim <[email protected]> * Update qiskit_experiments/library/characterization/tphi.py Co-authored-by: Yael Ben-Haim <[email protected]> * Removed two unnecessary methods, and did some cleaning following suggestions from review * Added class documentation. Removed overriding of shots * Removed unnecessary parameter that I previously added to BatchExperiment and CompositeExperiment constructors * Removed setting of shots in backend options. Changed 'block_for_results' to 'assertExperimentDone' * Fixed setting of default options in Tphi * Added links to classes in the release notes * Added info on the tphi experiment to the release notes * Implemented a set_experiment_options for tphi, that sets the options for the sub-experiments, without storing the options for the main object. Removed _default_experiment_options, that is no longer needed * Added test to check that circuits are modified correctly when using set_experiment_options after run * Added component analysis classes to the constructor of TphiAnalysis() following changes in PR 633 * Added setting of osc_freq to set_experiment_options. Added a respective test * Added to tests 'assertExperimentDone' Co-authored-by: Yael Ben-Haim <[email protected]>
Summary
New experiment Tphi. It computes the pure dephasing time and is computed by subtracting the effect of T1 from T2.
Details and comments
It is implemented here as a composite experiment with sub-experiments T1 and T2Ramsey.