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

Remove deprecated code for 0.7 #1452

Merged
merged 11 commits into from
May 24, 2024
3 changes: 3 additions & 0 deletions qiskit_experiments/curve_analysis/base_curve_analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ def _default_options(cls) -> Options:
fitting model. This is ``False`` by default.
plot (bool): Set ``True`` to create figure for fit result or ``False`` to
not create a figure. This overrides the behavior of ``generate_figures``.
return_fit_parameters (bool): (Deprecated) Set ``True`` to return all fit model parameters
with details of the fit outcome. Default to ``False``.
data_processor (Callable): A callback function to format experiment data.
This can be a :class:`.DataProcessor`
instance that defines the `self.__call__` method.
Expand Down Expand Up @@ -187,6 +189,7 @@ def _default_options(cls) -> Options:
options.plotter = CurvePlotter(MplDrawer())
options.plot_raw_data = False
options.plot_residuals = False
options.return_fit_parameters = True
options.data_processor = None
options.normalization = False
options.average_method = "shots_weighted"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,16 @@ def _default_options(cls) -> Options:
the analysis result.
plot (bool): Set ``True`` to create figure for fit result.
This is ``True`` by default.
return_fit_parameters (bool): (Deprecated) Set ``True`` to return all fit model parameters
with details of the fit outcome. Default to ``False``.
extra (Dict[str, Any]): A dictionary that is appended to all database entries
as extra information.
"""
options = super()._default_options()
options.update_options(
plotter=CurvePlotter(MplDrawer()),
plot=True,
return_fit_parameters=False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return_fit_parameters=False,
return_fit_parameters=True,

I think this has been True because options value comes from the base class. For example Ham tomo experiment has returned the fit parameter (in the unittest we just don't search by index)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I ran ham tomo on current code but didn't see the fit parameter:
image
You think we should change this behavior? I don't remember why return_fit_parameters was set to false for composite curve analysis.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I realized we unintentionally introduced breaking API change in the previous release.
https://github.com/Qiskit-Extensions/qiskit-experiments/blob/bc0b89e0eeedfabab7de2d5b6c5472e26bdbfb5a/qiskit_experiments/curve_analysis/composite_curve_analysis.py#L373-L375
We should have duplicated this combined_summary in the analysis results, but we only added this to artifact of the composite curve analysis. So the data has been moved to artifact without any warning. So far we didn't see any report in our issue, and I think this bug is not enough serious to motivate us to make a patch release for 0.6. And we don't need to redefine return_fit_parameters in the composite curve analysis options. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thanks for finding the source of the problem. How about I just keep the code the same and add a note in the Known Issues section of the release notes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me!

extra={},
)

Expand Down Expand Up @@ -302,7 +305,9 @@ def _run_analysis(
analysis = source_analysis.copy()
metadata = analysis.options.extra
metadata["group"] = analysis.name
analysis.set_options(plot=False, extra=metadata)
analysis.set_options(
plot=False, extra=metadata, return_fit_parameters=self.options.return_fit_parameters
)
results, _ = analysis._run_analysis(experiment_data)
for res in results:
if isinstance(res, ArtifactData):
Expand Down
19 changes: 10 additions & 9 deletions qiskit_experiments/curve_analysis/curve_analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -638,15 +638,16 @@ def _run_analysis(
# to generate the figure
plot_bool = plot == "always" or (plot == "selective" and quality == "bad")

# Store fit status overview entry regardless of success.
# This is sometime useful when debugging the fitting code.
overview = AnalysisResultData(
name=PARAMS_ENTRY_PREFIX + self.name,
value=fit_data,
quality=quality,
extra=self.options.extra,
)
result_data.append(overview)
if self.options.return_fit_parameters:
# Store fit status overview entry regardless of success.
# This is sometime useful when debugging the fitting code.
overview = AnalysisResultData(
name=PARAMS_ENTRY_PREFIX + self.name,
value=fit_data,
quality=quality,
extra=self.options.extra,
)
result_data.append(overview)

if fit_data.success:
# Add fit data to curve data table
Expand Down
20 changes: 17 additions & 3 deletions qiskit_experiments/framework/experiment_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -2674,11 +2674,18 @@ def delete_artifact(
"""Delete specified artifact data.

Args:
artifact_key: UID, name or index of the figure.
artifact_key: UID or name of the artifact. Deleting by index is deprecated.

Returns:
Deleted artifact ids.
"""
if isinstance(artifact_key, int):
warnings.warn(
"Accessing artifacts via a numerical index is deprecated and will be "
"removed in a future release. Use the ID or name of the artifact "
"instead.",
DeprecationWarning,
)
artifact_keys = self._find_artifact_keys(artifact_key)

for key in artifact_keys:
Expand All @@ -2696,14 +2703,21 @@ def artifacts(
"""Return specified artifact data.

Args:
artifact_key: UID, name or index of the figure.
artifact_key: UID or name of the artifact. Supplying a numerical index
is deprecated.

Returns:
A list of specified artifact data.
"""
if artifact_key is None:
return self._artifacts.values()

elif isinstance(artifact_key, int):
warnings.warn(
"Accessing artifacts via a numerical index is deprecated and will be "
"removed in a future release. Use the ID or name of the artifact "
"instead.",
DeprecationWarning,
)
artifact_keys = self._find_artifact_keys(artifact_key)

out = []
Expand Down
8 changes: 4 additions & 4 deletions releasenotes/notes/0_6_deprecations-9a399c48c2d461f1.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ upgrade_package:
* The ``return_data_points`` option has been removed from curve analysis.
Data points are now automatically provided in :class:`.ExperimentData` objects via the ``curve_data``
artifact.
* The ``return_fit_parameters`` option has been removed from curve analysis.
Fit parameters are automatically provided in the ``fit_summary`` artifact
of :class:`.ExperimentData` and will be removed from analysis results in a
future release.
* The default value of ``flatten_results`` in composite experiments has changed to ``True``.
deprecations:
- |
* Accessing experiment data artifacts by numerical index has been
deprecated. Use the name or ID of the artifact instead.
12 changes: 3 additions & 9 deletions test/database_service/test_db_experiment_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -1223,7 +1223,6 @@ def test_add_delete_artifact(self):
self.assertEqual(exp_data.artifacts(), [])
new_artifact = ArtifactData(name="test", data="foo")
exp_data.add_artifacts(new_artifact)
self.assertEqual(exp_data.artifacts(0), new_artifact)
self.assertEqual(exp_data.artifacts("test"), new_artifact)

service = mock.create_autospec(IBMExperimentService, instance=True)
Expand All @@ -1236,8 +1235,6 @@ def test_add_delete_artifact(self):
exp_data.delete_artifact("test")
self.assertEqual(exp_data.artifacts(), [])
self.assertEqual(exp_data._deleted_artifacts, {"test"})
with self.assertRaises(ExperimentEntryNotFound):
exp_data.artifacts(0)

exp_data.save()
# after saving, artifact_files should be updated again
Expand All @@ -1250,9 +1247,9 @@ def test_add_delete_artifact(self):
self.assertEqual(exp_data.artifacts(), [new_artifact, new_artifact2, new_artifact3])
self.assertEqual(exp_data.artifacts("test"), [new_artifact, new_artifact2])

deleted_id = exp_data.artifacts(0).artifact_id
deleted_id = exp_data.artifacts()[0].artifact_id
# delete by index
exp_data.delete_artifact(0)
exp_data.delete_artifact(deleted_id)

self.assertEqual(exp_data.artifacts(), [new_artifact2, new_artifact3])
with self.assertRaises(ExperimentEntryNotFound):
Expand All @@ -1266,7 +1263,7 @@ def test_add_delete_artifact(self):

# finish deleting artifacts named test
# delete by id
exp_data.delete_artifact(exp_data.artifacts(0).artifact_id)
exp_data.delete_artifact(exp_data.artifacts()[0].artifact_id)
self.assertEqual(exp_data.artifacts(), [new_artifact3])
exp_data.save()
self.assertEqual(exp_data._deleted_artifacts, set())
Expand Down Expand Up @@ -1296,8 +1293,5 @@ def test_delete_nonexistent_artifact(self):
new_artifact1 = ArtifactData(artifact_id="0", name="test", data="foo")
exp_data.add_artifacts(new_artifact1)

with self.assertRaises(ExperimentEntryNotFound):
exp_data.delete_artifact(2)

with self.assertRaises(ExperimentEntryNotFound):
exp_data.delete_artifact("123")
Loading