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

Update copy analysis result and experiment data behavior #1432

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions qiskit_experiments/framework/analysis_result_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,16 +204,26 @@ def clear(self):
with self._lock:
self._data = pd.DataFrame(columns=self.DEFAULT_COLUMNS)

def copy(self):
def copy(self, new_ids: bool = True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing uses new_ids=False, so should we just not include that option for now? It's a question of why a user is calling copy. If making a copy, it seems likely the user would want to change something and then in that case likely that new IDs should be used?

"""Create new thread-safe instance with the same data.

.. note::
This returns a new object with shallow copied data frame.
Args:
new_ids: Whether to generate new IDs for copied entries. Defaults to True.

Returns:
A new shallow copied DataFrame object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the use of DataFrame here because the DataFrame is an internal container of the data not directly exposed to the user. Also, maybe the point about being shallow is better in a .. note:: block like it was and here it could just say A copy of the analysis result table? I think the original note was not clear enough. The default columns of the data frame are all strings and numbers which are all immutable so the only difference between being shallow and deep is saving some memory. However, I think it's possible that the user could add a column with mutable objects? The concern there is that then mutating the objects in the copied table would also mutate the ones in the original. Maybe this is a pretty minor concern -- would a user add mutable objects to an analysis result table?

"""
with self._lock:
# Hold the lock so that no data can be added
new_instance = self.__class__()
new_instance._data = self._data.copy(deep=False)
if new_ids:
new_instance._data["result_id"] = None
for idx, _ in new_instance._data.iterrows():
new_instance._data.at[idx, "result_id"] = new_instance._create_unique_hash()
new_instance._data.index = [
result_id[:8] for result_id in new_instance._data["result_id"]
]
return new_instance

def _create_unique_hash(self) -> str:
Expand Down
5 changes: 4 additions & 1 deletion qiskit_experiments/framework/experiment_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -2397,7 +2397,10 @@ def copy(self, copy_results: bool = True) -> "ExperimentData":
# Copy results and figures.
# This requires analysis callbacks to finish
self._wait_for_futures(self._analysis_futures.values(), name="analysis")
new_instance._analysis_results = self._analysis_results.copy()
copied_results = self._analysis_results.copy()
# Analysis results should have experiment ID of the copied experiment
copied_results._data["experiment_id"] = new_instance.experiment_id
new_instance._analysis_results = copied_results
with self._figures.lock:
new_instance._figures = ThreadSafeOrderedDict()
new_instance.add_figures(self._figures.values())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
Fixed a bug where a copied :class:`.ExperimentData` instance's analysis results didn't get
new IDs and experiement ID.
15 changes: 15 additions & 0 deletions test/database_service/test_db_experiment_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
"""Test ExperimentData."""
from test.base import QiskitExperimentsTestCase
from test.fake_experiment import FakeExperiment
from test.extended_equality import is_equivalent

import os
from unittest import mock
import copy
Expand Down Expand Up @@ -1058,12 +1060,25 @@ def test_additional_attr(self):
def test_copy_metadata(self):
"""Test copy metadata."""
exp_data = FakeExperiment(experiment_type="qiskit_test").run(backend=FakeBackend())
self.assertExperimentDone(exp_data)
exp_data.add_data(self._get_job_result(1))
copied = exp_data.copy(copy_results=False)
self.assertEqual(exp_data.data(), copied.data())
self.assertFalse(copied.analysis_results())
self.assertEqual(exp_data.provider, copied.provider)

def test_copy_analysis_results(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good. Should there be a test of copy in test_analysis_results_table.py as well?

"""Test copy analysis results."""
exp_data = FakeExperiment(experiment_type="qiskit_test").run(backend=FakeBackend())
self.assertExperimentDone(exp_data)
exp_data.add_data(self._get_job_result(1))
copied = exp_data.copy(copy_results=True)
for res in copied.analysis_results():
self.assertEqual(res.experiment_id, copied.experiment_id)
# copied analysis results should be identical to the original except for experiment ID
copied._analysis_results._data["experiment_id"] = exp_data.experiment_id
self.assertTrue(is_equivalent(exp_data.analysis_results(), copied.analysis_results()))

def test_copy_figure_artifacts(self):
"""Test copy expdata figures and artifacts."""
exp_data = FakeExperiment(experiment_type="qiskit_test").run(backend=FakeBackend())
Expand Down