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

Do not use the deprecated matplotlib config option 'text.latex.preview'. #5233

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

csadorf
Copy link
Contributor

@csadorf csadorf commented Nov 17, 2021

Deprecated as of version 3.3.4, see:
https://matplotlib.org/3.3.4/api/api_changes.html#text-latex-preview-rcparam

Requires matplotlib~=3.3.

Fixes #5231.

@csadorf csadorf marked this pull request as draft November 17, 2021 12:17
@csadorf
Copy link
Contributor Author

csadorf commented Nov 17, 2021

Alternative to #5232 .

@csadorf csadorf added the dependencies Pull requests that update a dependency file label Nov 17, 2021
@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #5233 (a3fae75) into develop (01a8846) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5233   +/-   ##
========================================
  Coverage    81.23%   81.23%           
========================================
  Files          533      533           
  Lines        37356    37356           
========================================
  Hits         30344    30344           
  Misses        7012     7012           
Flag Coverage Δ
django 76.11% <ø> (ø)
sqlalchemy 75.19% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/orm/nodes/data/array/bands.py 77.46% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01a8846...a3fae75. Read the comment docs.

@csadorf csadorf marked this pull request as ready for review November 17, 2021 12:57
@csadorf
Copy link
Contributor Author

csadorf commented Nov 17, 2021

@sphuber All tests pass with this patch, I am just not 100% sure whether there are any side-effects that are not captured by the tests. I will add @giovannipizzi as reviewer, since he originally authored this particular code segment.

@sphuber
Copy link
Contributor

sphuber commented Nov 17, 2021

Thanks for the PR @csadorf . Would indeed be good to have @giovannipizzi give a look as I have never worked on nor used this code. Looking at the comment in our code and the deprecation message from matplotlib, it seems this option was necessary to provide correct alignment when using the TeX output format, but as of more recent matplotlib versions, that is done automatically. The ideal solution then would maybe to just have a conditional import. If an older version of matplotlib is installed, use the deprecated option, otherwise don't bother. Not sure how often this code is used though and so whether it is worth the effort. Pretty sure that the tests don't explicitly check this, so very possible that removing it will lead to incorrect images with older versions of matplotlib

@csadorf
Copy link
Contributor Author

csadorf commented Nov 17, 2021

The ideal solution then would maybe to just have a conditional import. If an older version of matplotlib is installed, use the deprecated option, otherwise don't bother. Not sure how often this code is used though and so whether it is worth the effort. Pretty sure that the tests don't explicitly check this, so very possible that removing it will lead to incorrect images with older versions of matplotlib

I've added the matplotlib~=3.3 dependency with this patch, so I don't think there is a need need for a conditional import. However, I am just realizing that we should probably add ,>=3.4.4 to ensure that we are actually getting the right version.

@csadorf csadorf force-pushed the fix/issue-5231-fix-incompatibility branch from 4c2cc45 to a3fae75 Compare November 17, 2021 15:08
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

I couldn't test it, but I'd say it's OK to remove it!
Thanks

@csadorf csadorf merged commit 632ab72 into develop Nov 18, 2021
@csadorf csadorf deleted the fix/issue-5231-fix-incompatibility branch November 18, 2021 07:14
@csadorf
Copy link
Contributor Author

csadorf commented Nov 18, 2021

@chrisjsewell Alerting you to this fix which might warrant a hotfix release.

sphuber pushed a commit that referenced this pull request Mar 7, 2022
sphuber pushed a commit that referenced this pull request Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Newly released matplotlib 3.5.0 breaks tests
3 participants