Skip to content

Deprecation fix mpl 3.10 and beyond #69

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

Merged
merged 72 commits into from
Apr 28, 2025
Merged

Conversation

cvanelteren
Copy link
Contributor

There are a lot of deprecation warnings from MPL and or Ultraplot existing in the current build. The majority of these fixes can be addressed by modifying the tests. Some of the deprecation warnings ensures keywords of usage to be compatible with mpl 3.10 which will result in errors in later versions.

There are a few errors I have not addressed that warrants some discussion.

  • The first one
    ultraplot/tests/test_1dplots.py::test_boxplot_vectors
    /home/casper/micromamba/envs/ultraplot-dev/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: UltraPlotWarning: Passing lists to fillalpha was removed in v0.9. Please specify different opacities using the property cycle colors instead.
        store.return_value[test_name] = obj(*args, **kwargs)
  • The second one
    ultraplot/tests/test_subplots.py::test_axis_sharing[labels]
    /home/casper/micromamba/envs/ultraplot-dev/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: UltraPlotWarning: Calling arbitrary axes methods from SubplotGrid was deprecated in v0.8 and will be removed in a future release. Please index the grid or loop over the grid instead.
    store.return_value[test_name] = obj(*args, **kwargs)

For both of these, I don't agree with the deprecation warning. The first issue indicates that we should move towards a cycler object. However, adjusting the subplot to match the target baseline cannot be readily achieved through the cycler object. It requires setting the patches to a different alpha and leaving the lines with alpha = 1. This would (I think) require an extension of what the cycler object can handle which is to set all potential properties of all the artists. I don't think it can currently do this.

The second error implies the removal of multi-axis formatting. Personally, I would be against removing this feature as it is one of the most powerful features of UltraPlot.

@cvanelteren cvanelteren marked this pull request as draft February 3, 2025 04:58
@cvanelteren
Copy link
Contributor Author

Making this a draft as we have not updated to 3.10 yet.

@cvanelteren
Copy link
Contributor Author

Note I had to rename the warning UltraplotWarning to UltraPlotWarning due to some weird pytest-xdist issue.

@cvanelteren cvanelteren added the TODO TODO label Feb 3, 2025
@cvanelteren
Copy link
Contributor Author

@beckermr

I had some spare time so pushed this along. There is one warning I could use your input for. There is a warning for the boxplot that keyword fillalphas is removed in 0.9 for proplot. I cannot determined directly why this was deprecated from the source but I have a hunch.

The warning implies that properties have to be fed through the Cycle object. However, the source is currently geared towards dealing with line properties only, setting color, alpha and marker style accordingly. This creates a stress point as the deprecation warning is not compatible with the current implementation of the source.

The major problem is that it is not trivial to determine from a singular entry point how to cycle general objects. Line objects have different properties from boxplots, and contours. As such, setting all the properties through a general cycler won't be possible unless we write the code for this. There are some properties that can be leveraged for this in ultraplot.internals.__init__.py that categorize the properties for the major artist types.

In short, we either
a) allow for the fillalphas to remain
b) extend the cycler object to allow cycling for different parts of the plot. For example we could have an internal map to line, patch, fliers, medians etc, and cycle the properties accordingly.

For b however, retooling would be necessary that will touch much of the codebase and I a don't know what will be gained tbh. I could be missing the point of the warnings and Luke's intentions, hence this msg to get some input on this issue. Happy to hear your thoughts.

P.S. This warning can be found by running ultraplot/tests/test_1dplots.py::test_boxplot_vectors (for example).

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I left some comments.

@cvanelteren
Copy link
Contributor Author

Hi @beckermr thanks for the comment but they don't address the things I outlined.

@beckermr
Copy link
Collaborator

Yes I know, sorry. I don't fully understand why things were deprecated either. I think not deprecating it fow now is fine.

@cvanelteren cvanelteren force-pushed the dep-fix branch 3 times, most recently from e9227f7 to 6a45e83 Compare April 8, 2025 12:21
@cvanelteren
Copy link
Contributor Author

Tests look as they should and fail on newly added once or correct existing errors.

@cvanelteren
Copy link
Contributor Author

Can we go ahead an merge this @beckermr?

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I had a few more comments, plus there are test failures in the plots that do not look like we expect them.

@beckermr
Copy link
Collaborator

Also the docs build is failing.

@cvanelteren
Copy link
Contributor Author

Will fix this and perhaps split this PR up for clarity.

cvanelteren and others added 8 commits April 28, 2025 10:08
* Introduces new rc param. Th parameter `colorbar.outline` defaults to `True` and toggles the outline frame of the colorbar. 
* Allows figure colorbar label to be put on arbitrary side of the figure.


---------
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Matthew R. Becker <[email protected]>
Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

This continue statement needs more discussion.

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Code looks good, though I do not understand all of the changes.

The test failures need fixing, and then I think we can merge.

@cvanelteren
Copy link
Contributor Author

Ok merging this as the newer plots more correct. We may need to revert the draw in fig.show as it caused the image sizes to differ.

@cvanelteren cvanelteren merged commit c4a399b into Ultraplot:main Apr 28, 2025
11 of 15 checks passed
@cvanelteren cvanelteren deleted the dep-fix branch April 28, 2025 10:55
@beckermr
Copy link
Collaborator

I was about to comment that the newer plots were more correct as well.

If you want code review, please wait until the reviewer approves before merging.

cvanelteren added a commit to cvanelteren/ultraplot that referenced this pull request Apr 28, 2025
*  Preparing for mpl 3.10

---------
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Matthew R. Becker <[email protected]>
@cvanelteren
Copy link
Contributor Author

Ah ok I gathered it was ok. Will wait next time.

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