-
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
Update BaseCalibrationExperiment for composite experiment #1033
Update BaseCalibrationExperiment for composite experiment #1033
Conversation
@@ -148,6 +148,37 @@ def calibrations(self) -> Calibrations: | |||
"""Return the calibrations.""" | |||
return self._cals | |||
|
|||
@property | |||
def analysis(self) -> Union[BaseAnalysis, 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.
What's the reason to redefine here, if already defined in BaseExperiment
?
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.
We can also do @BaseExperiment.analysis.setter
but we cannot override only setter in the subclass as far as I understand.
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.
This looks good to me. Only minor comments. Also, thanks for adding the tests!
|
||
@analysis.setter | ||
def analysis(self, analysis: Union[BaseAnalysis, None]) -> None: | ||
"""Set the analysis instance for the experiment""" |
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 add a bit of doc here to state that this is here so that update calibrations is called and so that this also works with composite 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.
note section is added to the property method rather than here. I think setter method doesn't appear in the API doc and one needs to read the code anyways to find the comment. Note that such comment is already in the method. Now note will appear with the updated property method.
releasenotes/notes/fix-missing_calibration_updator_call-a255b28dd1449ea4.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel J. Egger <38065505+eggerdj@users.noreply.github.com>
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. Thanks!
Summary
This PR updates the mechanism to invoke
update_calibrations
method so that it can be called from the composite experiment.fix #969
Details and comments
Composite experiment only takes circuits from individual experiment instance and calls analysis.run of each experiment. If
update_calibrations
is called from experiment.run it will be never called from the composite experiments. This indicates we need to formally implement post analysis or chained analysis to update calibrations. However, this is overkill just to update calibrations. This PR implements minimum logic to invoke the updator as a part of analysis.run.Since unittest coverage is almost zero for composite calibration experiments, I newly added several unittests.