Define BaseExperiment._transpiled_circuits and deprecate BaseExperiment._postprocess_transpiled_circuits #677

kevinsung · 2022-02-17T16:18:08Z

Fixes #669.

Summary

Adds BaseExperiment._transpiled_circuits to enable customization of transpilation. This makes BaseExperiment._postprocess_transpiled_circuits obsolete so that is deprecated with a version 0.4.0 deadline.

Details and comments

@chriseclectic I didn't find any issues with composite experiments, but let me know if there are any specific tests I should add.

BaseExperiment._postprocess_transpiled_circuits

kevinsung · 2022-02-17T17:22:54Z

CI failure appears to be unrelated:

 Warning, treated as error:
Cell printed to stderr:
/home/runner/work/qiskit-experiments/qiskit-experiments/qiskit_experiments/curve_analysis/guess.py:188: RuntimeWarning: invalid value encountered in log
  coeffs = np.polyfit(x, np.log(y), deg=1)
/home/runner/work/qiskit-experiments/qiskit-experiments/qiskit_experiments/curve_analysis/curve_analysis.py:848: UserWarning: All initial guesses and parameter boundaries failed to fit the data. Please provide better initial guesses or fit parameter boundaries.
  warnings.warn(

nkanazawa1989

Code looks good to me but this is indeed conflicting with #667 @yaelbh could you please decide if merge this first or close this and make @kevinsung co-author of your PR.

qiskit_experiments/framework/base_experiment.py

nkanazawa1989 · 2022-02-17T18:16:41Z

qiskit_experiments/framework/base_experiment.py

-        """Additional post-processing of transpiled circuits before running on backend"""
-        pass
+    def _transpile(
+        self, circuits: List[QuantumCircuit], backend: Backend, **transpile_options


Probably backend is not necessary since you can access to it via self.backend (also this will bypass _set_backend hook). Transpile options and circuit too. Likely you want to make it static method -- is there any plan to hack this method? Something like injecting circuit which is not generated by this experiment instance?

It is necessary because self.backend could be None, in which case the backend is passed to BaseExperiment.run and forwarded to this function.

It should not be a static method because transpilation can depend on the experiment instance.

This is not correct because existence of backend is guaranteed by these lines. Seems like this allows a hack that injecting backend which is not associated with the instance. Currently you are using self to only access deprecated method.
https://github.com/Qiskit/qiskit-experiments/blob/317b964b8cbde0febcda8696fd4bd5cce3490e0a/qiskit_experiments/framework/base_experiment.py#L251-L259

Nothing needs to be passed as kwargs to this function except the circuits (and that one is debatable too, the only reason to pass them is to allow overloading with some pre-transpilation logic). Options and backend should all be taken from the self instance. As Naoki points out backend can never be None when this function is called inside run.

Perhaps you are assuming the situation the user directly call this method of the instance initialized without backend to check the physical circuit? Then I agree it's worth having backend here. But I think circuit and transpile options can come from self.

Still, the backend needs to be forwarded because it uses experiment.backend which could be different from self.backend.

Perhaps you are assuming the situation the user directly call this method of the instance initialized without backend to check the physical circuit? Then I agree it's worth having backend here. But I think circuit and transpile options can come from self.

Yes, exactly. If the user calls run with a backend specified, then using self.backend would be incorrect. I actually this this a bit confusing, and that only one way of passing the backend should be supported. That would also eliminate the need for the complicated backend selection code. But that is a separate issue.

qiskit_experiments/framework/base_experiment.py

yaelbh · 2022-02-17T18:44:19Z

I suggest that the PR that's ready first will be merged first, and the other one will resolve the conflicts

qiskit_experiments/framework/base_experiment.py

chriseclectic · 2022-02-17T19:01:06Z

qiskit_experiments/framework/base_experiment.py

-        """Additional post-processing of transpiled circuits before running on backend"""
-        pass
+    def _transpile(
+        self, circuits: List[QuantumCircuit], backend: Backend, **transpile_options


Nothing needs to be passed as kwargs to this function except the circuits (and that one is debatable too, the only reason to pass them is to allow overloading with some pre-transpilation logic). Options and backend should all be taken from the self instance. As Naoki points out backend can never be None when this function is called inside run.

qiskit_experiments/framework/base_experiment.py

kevinsung · 2022-02-17T19:27:05Z

@chriseclectic I have eliminated the transpilation options keyword arguments. As I explained above, backend is needed for the case the user called run with a backend specified. I think circuits should remain as a keyword argument in order to have a clean separation of functionality with BaseExperiment.circuits.

yaelbh

Thanks @kevinsung 😃.
I intentionally mark "Comment" and not "Request changes", to make the approvals of Chris and Naoki sufficient for merging the PR.

qiskit_experiments/framework/base_experiment.py

yaelbh · 2022-02-20T07:25:44Z

Please don't proceed with approving and merging this PR before we resolve #667 (comment).

kevinsung · 2022-02-21T14:16:29Z

I've updated the code to align with #667 by changing _transpile to _transpiled_circuits, which does not take the circuits as input.

kevinsung · 2022-02-22T14:16:25Z

CI failure appears unrelated again, see #682

chriseclectic

This looks very close. I would suggest merging #667 first, then rebasing this again after.

qiskit_experiments/framework/base_experiment.py

releasenotes/notes/transpile-617bd3a4e6f1c0d8.yaml

kevinsung · 2022-02-22T18:34:54Z

@chriseclectic I've addressed your comments. It might make sense to merge this before #667 because #667 actually includes this change and does more on top.

chriseclectic

Minor formatting

releasenotes/notes/transpile-617bd3a4e6f1c0d8.yaml

chriseclectic

@kevinsung sounds good, we can merge this one first

Comments addressed

Add BaseExperiment._transpile_circuits method and deprecate BaseExperiment._postprocess_transpiled_circuits Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>

kevinsung force-pushed the transpile branch 3 times, most recently from bda4650 to 07e0246 Compare February 17, 2022 16:40

define BaseExperiment._transpile and deprecate

5343775

BaseExperiment._postprocess_transpiled_circuits

kevinsung force-pushed the transpile branch from 07e0246 to 5343775 Compare February 17, 2022 16:43

nkanazawa1989 previously requested changes Feb 17, 2022

View reviewed changes

kevinsung mentioned this pull request Feb 17, 2022

BatchExperiment - defer transpilation to children #667

Merged

4 tasks

chriseclectic requested changes Feb 17, 2022

View reviewed changes

kevinsung added 2 commits February 17, 2022 14:15

construct transpilation options within _transpile

89d97d7

format

642ff3a

chriseclectic mentioned this pull request Feb 18, 2022

Transpile parallel experiment sub circuits #680

Merged

yaelbh reviewed Feb 20, 2022

View reviewed changes

qiskit_experiments/framework/base_experiment.py Outdated Show resolved Hide resolved

qiskit_experiments/framework/base_experiment.py Outdated Show resolved Hide resolved

kevinsung added 2 commits February 21, 2022 09:03

change _transpile to _transpiled_circuits

a755d09

use experiment instead of self

a9b7b06

kevinsung mentioned this pull request Feb 21, 2022

Deprecate setting backend in BaseExperiment.run #681

Closed

kevinsung changed the title ~~Define BaseExperiment._transpile and deprecate BaseExperiment._postprocess_transpiled_circuits~~ Define BaseExperiment._transpiled_circuits and deprecate BaseExperiment._postprocess_transpiled_circuits Feb 21, 2022

chriseclectic mentioned this pull request Feb 21, 2022

Combine PRs which all effect each other yaelbh/qiskit-experiments#4

Closed

kevinsung mentioned this pull request Feb 22, 2022

continuous integration is flaky #682

Closed

remove backend arg

62d198f

chriseclectic reviewed Feb 22, 2022

View reviewed changes

update release note

d1ff1bb

Merge branch 'main' into transpile

7dd85c9

chriseclectic requested changes Feb 23, 2022

View reviewed changes

releasenotes/notes/transpile-617bd3a4e6f1c0d8.yaml Outdated Show resolved Hide resolved

releasenotes/notes/transpile-617bd3a4e6f1c0d8.yaml Outdated Show resolved Hide resolved

releasenotes/notes/transpile-617bd3a4e6f1c0d8.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

51ed581

chriseclectic approved these changes Feb 23, 2022

View reviewed changes

chriseclectic merged commit d788c2a into qiskit-community:main Feb 23, 2022

kevinsung deleted the transpile branch February 23, 2022 18:31

Define BaseExperiment._transpiled_circuits and deprecate BaseExperiment._postprocess_transpiled_circuits #677

Define BaseExperiment._transpiled_circuits and deprecate BaseExperiment._postprocess_transpiled_circuits #677

Uh oh!

Conversation

kevinsung commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

kevinsung commented Feb 17, 2022

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinsung Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaelbh commented Feb 17, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinsung commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaelbh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yaelbh commented Feb 20, 2022

Uh oh!

kevinsung commented Feb 21, 2022

Uh oh!

kevinsung commented Feb 22, 2022

Uh oh!

chriseclectic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinsung commented Feb 22, 2022

Uh oh!

chriseclectic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chriseclectic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinsung commented Feb 17, 2022 •

edited

Loading

kevinsung Feb 17, 2022 •

edited

Loading

kevinsung commented Feb 17, 2022 •

edited

Loading