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

@@ -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)
 

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]:

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
+    <https://github.com/Qiskit-Extensions/qiskit-experiments/pull/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
+    <https://github.com/Qiskit-Extensions/qiskit-experiments/pull/1430>`__.

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)

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,30 @@ 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)
+
+        # 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()