From c3211f1feab9e9580ce019550ca2c76f9b07c597 Mon Sep 17 00:00:00 2001 From: Will Shanks Date: Fri, 22 Mar 2024 11:25:08 -0400 Subject: [PATCH] Fix inconsistent handling of figure names `ExperimentData._clear_results()` cleared `ExperimentData._figures` but not the related `ExperimentData._db_data.figure_names` exposed as `ExperimentData.figure_names`. `BaseAnalysis.run()` calls `ExperimentData._clear_results()` so when a second analysis run produced a different set of figure names from the first the `ExperimentData` instance would be left a `figure_names` property with entries that did not match the underlying figures data. Here, this issue is addressed by also clearing `ExperimentData._db_data.figure_names`. This issue was first noticed due to occasional failures of the `test.database_service.test_db_experiment_data.TestDbExperimentData.test_copy_figure_artifacts` test which runs a fake experiment and then adds a figure and artifact to the experiment data. Then the tests copies the experiment data to test the behavior of `copy()`. Occasionally, the adding of the figure occurs before the background analysis thread runs. In this case, the figure name gets added to `figure_names` before the results get cleared by the analysis callback. Because of the bug described above, the old figure name stays around in the original experiment data object but does not copy to the new object because figure names are copied using the data in `ExperimentData._figures` and not `ExperimentData._db_data.figure_names`. A `block_for_results()` was added to the experiment because in principle the `copy()` could occur before the analysis clears results and then the two data objects would not match, though this case has not been observed (only adding the figure and then clearing it with analysis before copying so the copy is the one missing the figure name has been observed; not the copy having the name and not the original). Addtionally, `BaseAnalysis.run()` handled the `figure_names` option incorrectly when passes a keyword argument. Keyword arguments to passed to `run()` cause the analysis class to copy itself and add the options to the copy before running it. However, within the `run()` method, `self.options.figure_names` was used for assigning figure names rather than referencing the copy of the analysis class, so figure names could not be set using `figure_names` as a keyword argument. This issues was fixed by replacing `self` references with references to the copy. A regression test was added that fails for either of the above issues being present (figure names not matching after re-running analysis; figure names not settable from a `run()` keyword argument). --- qiskit_experiments/framework/base_analysis.py | 6 ++--- .../framework/experiment_data.py | 1 + ...-names-inconsistency-afca1ac8e00fabac.yaml | 14 ++++++++++ .../test_db_experiment_data.py | 1 + test/framework/test_framework.py | 26 +++++++++++++++++++ 5 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/figure-names-inconsistency-afca1ac8e00fabac.yaml diff --git a/qiskit_experiments/framework/base_analysis.py b/qiskit_experiments/framework/base_analysis.py index 167e07e319..b0ca658418 100644 --- a/qiskit_experiments/framework/base_analysis.py +++ b/qiskit_experiments/framework/base_analysis.py @@ -184,7 +184,7 @@ def run_analysis(expdata: ExperimentData): if not result.experiment: result.experiment = expdata.experiment_type if not result.device_components: - result.device_components = self._get_experiment_components(expdata) + result.device_components = analysis._get_experiment_components(expdata) if not result.backend: result.backend = expdata.backend_name if not result.created_time: @@ -204,7 +204,7 @@ def run_analysis(expdata: ExperimentData): if not result.experiment_id: result.experiment_id = expdata.experiment_id if not result.device_components: - result.device_components = self._get_experiment_components(expdata) + result.device_components = analysis._get_experiment_components(expdata) if not result.experiment: result.experiment = expdata.experiment_type expdata.add_artifacts(result) @@ -227,7 +227,7 @@ def run_analysis(expdata: ExperimentData): name=f"{expdata.experiment_type}_{qubits_repr}_{short_id}.svg", ) figure_to_add.append(figure) - expdata.add_figures(figure_to_add, figure_names=self.options.figure_names) + expdata.add_figures(figure_to_add, figure_names=analysis.options.figure_names) experiment_data.add_analysis_callback(run_analysis) diff --git a/qiskit_experiments/framework/experiment_data.py b/qiskit_experiments/framework/experiment_data.py index 3bfe764718..62fe646aca 100644 --- a/qiskit_experiments/framework/experiment_data.py +++ b/qiskit_experiments/framework/experiment_data.py @@ -630,6 +630,7 @@ def _clear_results(self): self._deleted_figures.append(key) self._figures = ThreadSafeOrderedDict() self._artifacts = ThreadSafeOrderedDict() + self._db_data.figure_names.clear() @property def service(self) -> Optional[IBMExperimentService]: diff --git a/releasenotes/notes/figure-names-inconsistency-afca1ac8e00fabac.yaml b/releasenotes/notes/figure-names-inconsistency-afca1ac8e00fabac.yaml new file mode 100644 index 0000000000..f7066e4c1d --- /dev/null +++ b/releasenotes/notes/figure-names-inconsistency-afca1ac8e00fabac.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + :class:`.ExperimentData` was updated so that running analysis a second time + with ``replace_results=True`` does not result in the ``figure_names`` + property having incorrect data (both old and new figure names if the names + changed). See `#1430 + `__. + - | + :class:`.BaseAnalysis` was updated to respect ``figure_names`` as a keyword + argument to the ``run()`` method. Previously, this argument was ignored and + ``figure_names`` could only be set as an analysis option prior to calling + ``run()``. See `#1430 + `__. diff --git a/test/database_service/test_db_experiment_data.py b/test/database_service/test_db_experiment_data.py index 020e871cc7..d9347a6377 100644 --- a/test/database_service/test_db_experiment_data.py +++ b/test/database_service/test_db_experiment_data.py @@ -1067,6 +1067,7 @@ def test_copy_metadata(self): def test_copy_figure_artifacts(self): """Test copy expdata figures and artifacts.""" exp_data = FakeExperiment(experiment_type="qiskit_test").run(backend=FakeBackend()) + exp_data.block_for_results() exp_data.add_figures(str.encode("hello world")) exp_data.add_artifacts(ArtifactData(name="test", data="foo")) copied = exp_data.copy(copy_results=True) diff --git a/test/framework/test_framework.py b/test/framework/test_framework.py index 6d45fe2f67..85bfafcb00 100644 --- a/test/framework/test_framework.py +++ b/test/framework/test_framework.py @@ -28,6 +28,7 @@ from qiskit_experiments.exceptions import AnalysisError from qiskit_experiments.framework import ( ExperimentData, + FigureData, BaseExperiment, BaseAnalysis, AnalysisResultData, @@ -163,6 +164,31 @@ def test_analysis_replace_results_true(self): self.assertEqualExtended(expdata1.analysis_results(), expdata2.analysis_results()) self.assertEqual(result_ids, list(expdata2._deleted_analysis_results)) + def test_analysis_replace_results_true_new_figure(self): + """Test running analysis with replace_results=True keeps figure data consistent""" + analysis = FakeAnalysis() + analysis.options.add_figures = True + analysis.options.figure_names = ["old_figure_name.svg"] + + expdata = ExperimentData() + expdata.add_data(self.fake_job_data()) + analysis.run(expdata, seed=54321) + self.assertExperimentDone(expdata) + fig_names_orig = list(expdata.figure_names) + + # Assure all figure names map to valid figures + self.assertEqual(expdata.figure_names, ["old_figure_name.svg"]) + self.assertIsInstance(expdata.figure("old_figure_name"), FigureData) + + analysis.run( + expdata, replace_results=True, seed=12345, figure_names=["new_figure_name.svg"] + ) + self.assertExperimentDone(expdata) + + # Assure figure names have changed but are still valid + self.assertEqual(expdata.figure_names, ["new_figure_name.svg"]) + self.assertIsInstance(expdata.figure("new_figure_name"), FigureData) + def test_analysis_replace_results_false(self): """Test running analysis with replace_results=False""" analysis = FakeAnalysis()