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

Address several warnings raised during tests #1351

Merged
merged 19 commits into from
Jan 19, 2024

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Dec 20, 2023

This change rolls together several small changes to avoid warnings generated when running the tests.

Here is a summary of the changes:

  • Update use of datetime.utcnow() deprecated in Python 3.12
  • Concatenate pandas dataframes more carefully to avoid FutureWarning when concatenating an empty dataframe (see DEPR: concat ignoring empty objects pandas-dev/pandas#52532).
  • Adjust plotter tests to avoid warnings
    • Use supported options rather than arbitrary options to avoid unexpected option warning
    • Catch expected warning when passing an untrained discriminator to IQPlotter
  • Move the setting of the axis scale earlier in the logic of MplDrawer.format_canvas. format_canvas queries the limits of each axes object and uses those values. When no data set on a set of axes (an odd edge case), the default values depend on the scale (because the minimum for linear is 0 but for log is 0.1), so the scale needs to be set first.
  • Remove transpile options from FineDragCal (_transpiled_circuits for BaseCalibrationExperiment warns about and ignores transpile options)
  • Replace isinstance checks with pulse_type checks for calibration tests checking pulse shape
  • Replace qiskit.Aer usage with qiskit_aer
  • Set tolerance on cvxpy test to larger value, close to the tolerance achieved in practice. The routine was maxing out its iterations without achieving tolerance but still producing a close enough result for the test to pass. Increasing the tolerance avoids the max iterations warning and makes the test faster.
  • Rename TestLibrary to MutableTestLibrary. The class name starting with Test* causes pytest to warn about the class looking like a class holding test cases. pytest is not the main way we run the tests but it still seems best to avoid confusion between test case classes and test helper classes.
  • Catch warnings about insufficient trials in QuantumVolume tests using small numbers of trials.
  • Catch user and deprecation warnings about calibration saving and loading to csv files.
  • Convert existing calibration saving and loading tests from json to csv, leaving basic coverage of csv saving. A bug was found with json saving, resulting in one test being marked as an expected failure.
  • Set data on the ExperimentData for TestFramework.test_run_analysis_experiment_data_pickle_roundtrip to avoid a warning about an empty experiment data object. Other cases of this warning were addressed in 729014b but this test case was added afterward in a6c9e53.

In order to avoid warnings creeping in in the future, this change also makes any warnings emitted by qiskit-experiments code be treated as exceptions by the tests (with an exception for ScatterTable methods for which it is planned to remove the deprecation). Any expected warnings in tests can be handled by using assertWarns or warnings.catch_warnings.

As all the changes relate to tests (except the FineDragCal one which is a non-functional change to avoid a warning), no release note was added.

Calibration experiments currently override `_transpiled_circuits()` and
ignore transpile options so these options were not doing anything
besides generating a `UserWarning` when creating a `FineDragCal`
instance.
The default tolerance was leading to a warning about max iterations
being reached without reaching the target objective.
@wshanks
Copy link
Collaborator Author

wshanks commented Dec 20, 2023

The only remaining warnings generated by qiskit_experiments when running the tests are the CurveData backwards compatibility methods of ScatterTable. Since the API of ScatterTable is still under discussion, I didn't try to change those uses.

Currently we mark a test as failing if it raises a DeprecationWarning. I was wondering if we should also mark a test as failing if it raises any warning from the qiskit_experiments package. It is hard to control warnings from other packages but we should be able to control our own. If we want to test generating warnings, we can use assertWarns or warnings.catch_warnings to avoid specific cases of warnings failing the tests.

Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

Thanks for fixing all these warnings and the changes look good. Besides the QV warnings, there's one remaining from TestFramework.test_run_analysis_experiment_data_pickle_roundtrip: https://github.com/Qiskit-Extensions/qiskit-experiments/actions/runs/7482494338/job/20366191685?pr=1351#step:8:486
And one from TestPlotter .test_series_data_end_to_end: https://github.com/Qiskit-Extensions/qiskit-experiments/actions/runs/7482494338/job/20366191685?pr=1351#step:8:1082

I agree with failing the tests on warnings coming from qiskit_experiments. Do you want to do it in this PR or a new one?

@@ -101,7 +101,11 @@ def test_unit_scale(self, args):
def test_scale(self):
"""Test the xscale and yscale figure options."""
plotter = MockPlotter(MplDrawer(), plotting_enabled=True)
plotter.set_figure_options(xscale="quadratic", yscale="log")
plotter.set_figure_options(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outside the scope of this PR, but we should probably handle the case of a log scale with limit 0 gracefully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to address this here (see aed5d30). I was able to find a somewhat elegant solution by moving the setting of the axis scale earlier in the process (see the commit message). The tricky part is that the code wants to know the axis limits for the part where it tries to rescale the format (like add a k and hide the 000) but if you have axes that have no data in them matplotlib returns 0,1 by default. If the code didn't do that formatting scaling, I would just change it to not set limits when no user limits were specified so matplotlib could use its default limits.

When the axis scale is changed to "log", matplotlib changes the default
axis limits from (0, 1) to something around (0.1, 10). Since MplDrawer
got the limits and the set them back on the axes after setting the
scale, it was reapplying a minimum of 0 to the log scale plot, leading
to a user warning. matplotlib by default changes the limits to the range
of the data when data is added, so this case only came up when
generating a figure with no data.
@wshanks
Copy link
Collaborator Author

wshanks commented Jan 11, 2024

Interesting. I made warnings cause tests to fail and a few other warnings surfaced. I will address them. I wonder why I didn't see them before in the test log when running all the tests to check for warnings.

Also, catch both user and deprecation warnings for remaining csv
calibration saving tests.
@wshanks
Copy link
Collaborator Author

wshanks commented Jan 12, 2024

@coruscating I made the warnings into failures, addressed the extra warnings you mentioned, and handled the 0 case of log scale plots better. Along the way, I found a bug in the calibration save/load code (#1355).

Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

Thanks, the axis scale fix is a nice one. We could add a release note for it, but it was unlikely that a user would have encountered these warnings, so it's up to you. Either way this is good to go.

qiskit_experiments/visualization/drawers/mpl_drawer.py Outdated Show resolved Hide resolved
@wshanks wshanks enabled auto-merge January 19, 2024 14:18
@wshanks
Copy link
Collaborator Author

wshanks commented Jan 19, 2024

By the way, I think we need this now to fix the tests against Qiskit main. It looks like the test run is failing during test discovery now because of the removal of Aer in Qiskit/qiskit#11442.

@wshanks wshanks added this pull request to the merge queue Jan 19, 2024
Merged via the queue into qiskit-community:main with commit c536026 Jan 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants