Remove deprecated code for 0.7 #1452

coruscating · 2024-04-29T16:40:28Z

Removes code scheduled for removal in 0.7. For analysis results, dataframe is still set to False by default as True is a breaking change.

nkanazawa1989

Thanks @coruscating . Overall this looks good to me. Do you have any preference on change in dataframe option? It's scheduled in this RFC.
https://github.com/Qiskit/RFCs/blob/master/0007-experiment-dataframe.md#qiskit_experiments-v07

nkanazawa1989 · 2024-04-30T17:28:45Z

qiskit_experiments/curve_analysis/curve_analysis.py

-            result_data.append(overview)
+        # Store fit status overview entry regardless of success.
+        # This is sometime useful when debugging the fitting code.
+        overview = AnalysisResultData(


Why do we keep this object? Now it's stored in the artifact right? Is this due to the index change of the analysis results? I would turn default option value into None and issue the deprecation warning unless user doesn't explicitly set this to False (maybe too annoying though). With this change we'll lose the knob to trigger deprecation warning in future when we remove this object, i.e. we cannot raise the warning when Python list [0] is called for the analysis results.

We had some discussions last release about how to deprecate/remove this entry (#1342 (comment)). I think since we've had one release cycle with artifacts it's ok to remove this directly. What do you think @wshanks?

The reason we kept this before was because many tests used integer arguments to analysis_results() and I worried that that likely reflected a common pattern in user code as well. So removing the first result would change the integer indices of all other results.

I don't think it is that big of a burden to leave the overview code in for now. I think I would leave it in along with return_fit_parameters until either passing an integer or slice or passing dataframe=False to analysis_results() is removed. What is the timeline for those removals?

Maybe deprecation for

When search index is int type or None, and

When the result includes overview entry

it the reasonable approach?

@nkanazawa1989 we are already issuing deprecating warnings for int/slice search index and retrieving the overview entry:

qiskit-experiments/qiskit_experiments/framework/experiment_data.py

Lines 1569 to 1582 in fe4ccff

if index == 0 and tmp_df.iloc[0]["name"].startswith("@"):

warnings.warn(

"Curve fit results have moved to experiment artifacts and will be removed "

"from analysis results in a future release. Use "

'expdata.artifacts("fit_summary").data to access curve fit results.',

DeprecationWarning,

)

elif isinstance(index, (int, slice)):

warnings.warn(

"Accessing analysis results via a numerical index is deprecated and will be "

"removed in a future release. Use the ID or name of the analysis result "

"instead.",

DeprecationWarning,

)

Do you want to implement this differently?

Yeah, I'm fine with leaving it as is until we're ready to make the full change since artifacts (and figures) are also currently accessible via the integer key. I think it's awkward for the behavior to be different across these interfaces. What do you think @nkanazawa1989? Maybe we just deprecate retrieving artifacts by index in this release?

Sounds like a plan. Let's deprecate it as well in this release and remove all of them in 0.8.

@coruscating I see you added the deprecation. Can we also put back the return_fit_parameters option until we remove returning the overview completely?

Done, this is ready for another look.

The part touched on here looks good now. The rest of the PR looks good as well but I didn't review very closely and will leave it to @nkanazawa1989.

nkanazawa1989

One minor change request and the rest of change looks fine. I'll merge this PR once suggested change is merged.

nkanazawa1989 · 2024-05-20T13:39:53Z

qiskit_experiments/curve_analysis/composite_curve_analysis.py

            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,


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)

Hmm I ran ham tomo on current code but didn't see the fit parameter:

You think we should change this behavior? I don't remember why return_fit_parameters was set to false for composite curve analysis.

I realized we unintentionally introduced breaking API change in the previous release.

qiskit-experiments/qiskit_experiments/curve_analysis/composite_curve_analysis.py

Lines 373 to 375 in bc0b89e

combined_summary = dict(sub_artifacts["fit_summary"])

artifacts.append(ArtifactData(name="fit_summary", data=combined_summary))

total_quality = self._evaluate_quality(combined_summary)

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?

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?

Sounds good to me!

nkanazawa1989

LGTM. Thanks.

coruscating added 6 commits April 9, 2024 13:40

remove deprecated code and update docs

d4dc486

set flatten_results to True and remove old provider

5edf036

revert provider removal

ff2089a

revert dataframe true by default

54f2111

merge main

5a96ca0

update release note

351af66

coruscating added this to the Release 0.7 milestone Apr 30, 2024

nkanazawa1989 requested review from nkanazawa1989 April 30, 2024 14:49

update documentation and lint

23607ce

nkanazawa1989 reviewed Apr 30, 2024

View reviewed changes

coruscating added 3 commits May 16, 2024 10:08

deprecate artifact index

3e41f83

Merge remote-tracking branch 'upstream/main' into 0.7_deprecations

9b6de29

reverts return_fit_parameters removal and update release note

6fa6420

nkanazawa1989 requested changes May 20, 2024

View reviewed changes

add note on composite fit params

8e7477c

coruscating requested a review from nkanazawa1989 May 23, 2024 23:08

nkanazawa1989 approved these changes May 24, 2024

View reviewed changes

nkanazawa1989 added this pull request to the merge queue May 24, 2024

Merged via the queue into Qiskit-Extensions:main with commit ce485f8 May 24, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated code for 0.7 #1452

Remove deprecated code for 0.7 #1452

coruscating commented Apr 29, 2024 •

edited

nkanazawa1989 left a comment

nkanazawa1989 Apr 30, 2024

coruscating May 1, 2024

wshanks May 1, 2024

nkanazawa1989 May 15, 2024

coruscating May 15, 2024

coruscating May 15, 2024

nkanazawa1989 May 16, 2024

wshanks May 16, 2024 •

edited

coruscating May 16, 2024

wshanks May 16, 2024

nkanazawa1989 left a comment

nkanazawa1989 May 20, 2024

coruscating May 20, 2024

nkanazawa1989 May 22, 2024

coruscating May 22, 2024

nkanazawa1989 May 23, 2024

nkanazawa1989 left a comment

	if index == 0 and tmp_df.iloc[0]["name"].startswith("@"):
	warnings.warn(
	"Curve fit results have moved to experiment artifacts and will be removed "
	"from analysis results in a future release. Use "
	'expdata.artifacts("fit_summary").data to access curve fit results.',
	DeprecationWarning,
	)
	elif isinstance(index, (int, slice)):
	warnings.warn(
	"Accessing analysis results via a numerical index is deprecated and will be "
	"removed in a future release. Use the ID or name of the analysis result "
	"instead.",
	DeprecationWarning,
	)

	combined_summary = dict(sub_artifacts["fit_summary"])
	artifacts.append(ArtifactData(name="fit_summary", data=combined_summary))
	total_quality = self._evaluate_quality(combined_summary)

Remove deprecated code for 0.7 #1452

Remove deprecated code for 0.7 #1452

Conversation

coruscating commented Apr 29, 2024 • edited

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wshanks May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

coruscating commented Apr 29, 2024 •

edited

wshanks May 16, 2024 •

edited