Run hooks to base framework (part of #380) #404
Conversation
nkanazawa1989
requested review from
chriseclectic,
eggerdj,
eliarbel and
yaelbh
chriseclectic
left a comment
chriseclectic
left a comment
There was a problem hiding this comment.
One question I have with this approach is adding these hooks in it becomes less clear which method can have side effects on the Experiment objects themselves, and what should happen in cases where you run circuits but perform analysis later (eg if loading saved data to analyze), or run multiple types to generate different datasets.
Previously circuits, run and run_analysis typically shouldn't have had any side effects on the instance so could be called repeatedly without issue. It looks like adding these callbacks changes this so it should perhaps be made clear what is going on now, and if they are intended to have side effects or not.
| """ | ||
| return circuits | ||
|
|
||
| def run_transpile(self, backend: Backend, **options) -> List[QuantumCircuit]: |
There was a problem hiding this comment.
I don't think this is a good name for this method. This isn't just doing run transpile, this is doing full circuit generation and the pre and post transpile actions along with transpilation. This is why I suggested calling it circuits in your original PR. Maybe run_circuits is better since it can potentially have side effects now with the pre and post actions, while the original circuits one didn't.
There was a problem hiding this comment.
I see your point. I don't think they do have side effect, because users don't directly submit output of circuits method to the backend. I assume this is mainly used to check what is executed (I'm fine with making it protected). I feel circuits and run_circuits are confusing, so the options would be
-
Turning current
circuitsinto a protected method and replacingcircuitsmethod withrun_transpile. This means one cannot check logical circuits. The output ofcircuitswill be transpiled and usually they look very complicated. -
Assigning completely different name to
run_transpile. Something likecompile_experimentorprepare_execution?
How do these options sound?
There was a problem hiding this comment.
Done in 3b6b372. Basically run_transpile functionality is integrated into circuits method and the original circuits method is now renamed to _circuits. New circuits method has an option run_transpile defaults to False, so by default, it returns logical circuits as before.
| """ | ||
| pass | ||
|
|
||
| def run_analysis(self, experiment_data: ExperimentData, **options) -> ExperimentData: |
There was a problem hiding this comment.
I think this method should be the one that calls _post_analysis_action, not the Analysis.run method. Since if this may have side effects on this class this should be clear form this class.
There was a problem hiding this comment.
Yes, that is what I originally implemented and people suggested me to exclude the post action from this method. However, this requires composite experiment run method to have boilerplate code because they need to call post analysis of each nested experiment. I think we can merge #407 first and revert the change based on #407.
There was a problem hiding this comment.
Now post analysis is called like this
https://github.com/Qiskit/qiskit-experiments/blob/ee6556ba13703a4281191aa868aa679cab0118e4/qiskit_experiments/framework/base_experiment.py#L138-L140
There was a problem hiding this comment.
I noticed above induces side effect in this situation 0c8b915
What happens if we call add_analysis_callback from run_analysis? Or should we just call post analysis without adding it to the experiment data callbacks?
…ure/experiment_run_hook_simple_base
- circuits is renamed to _circuits - run_transpile options is added to circuits
|
Closes #151 |
|
It's quite difficult to follow the discussion here. Where do things stand? |
4 participants
Summary
This is a part of PR #380. Update of composite experiment is dropped for review.
Details and comments