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

Fix unnecessary expdata copy for composite exps #601

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

chriseclectic
Copy link
Collaborator

Summary

Fixes check in BaseAnalysis for whether the ExperimentData container requires copying to avoid an unnecessary copy for empty composite experiment data containers.

Details and comments

Adds debug msg to logger so running with log level DEBUG will show when experiment data is copied

@chriseclectic chriseclectic added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jan 10, 2022
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM just minor question about update of test file.

@@ -186,11 +186,11 @@ def test_analysis_replace_results_true(self):

# Additional data not part of composite experiment
exp3 = FakeExperiment([0, 1])
extra_data = exp3.run(FakeBackend())
extra_data = exp3.run(FakeBackend()).block_for_results()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this relevant to copy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was just to be safe that the child fake experiment finishes all its analysis before later re-running analysis with replacement on the data.

Verified

This commit was signed with the committer’s verified signature.
Byron Sebastian Thiel
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM

@chriseclectic chriseclectic merged commit 90b0186 into qiskit-community:main Jan 21, 2022
@chriseclectic chriseclectic deleted the analysis-copy branch March 3, 2022 22:44
paco-ri pushed a commit to paco-ri/qiskit-experiments that referenced this pull request Jul 11, 2022
Fix unnecessary experiment data copy that occured when running composite analysis on composite experiment data that had not yet been analyzed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants