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.

Sign up for GitHub

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

Jump to bottom

Removal of matplotlib<3 compatibility checks #2665

Merged
merged 7 commits into from Jul 10, 2020
Merged

Removal of matplotlib<3 compatibility checks #2665

merged 7 commits into from Jul 10, 2020

Conversation

heavelock
Copy link
Contributor

I noticed a few TODOs to remove some compatibility checks when minimum matplotlib version will be higher than 2.0. Since we have minimum of 3.2 now, I went ahead and removed them. I also searched the codebase for all checks agains MATPLOTLIB_VERSION and I removed all that were checking for versions <3.0. There are left a few of them that are checking for MATPLOTLIB_VERSION == [3, 0, 1] but since we still support 3 as a major version, I didn't touch them.

barsch
barsch approved these changes Jul 1, 2020
View reviewed changes
Copy link
Member

@barsch barsch left a comment •
edited

Choose a reason for hiding this comment

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

looks good for me

those MATPLOTLIB_VERSION == [3, 0, 1] are all related to basemap issues which needs to be fixed later anyway

@heavelock
Copy link
Contributor Author

Yeah, I think the plan is to drop the basemap completely but for now I just figured out to keep it.

@barsch barsch requested a review from megies July 1, 2020 14:52
@heavelock
Copy link
Contributor Author

O, I caused few new errors on Flake8, I will fix them in a minute.

megies
megies approved these changes Jul 1, 2020
View reviewed changes
Copy link
Member

@megies megies left a comment

Choose a reason for hiding this comment

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

Looks good to me. We might need some reltol tweaks again later maybe.. but I guess we can readd it when/if needed. 👍

@heavelock
Copy link
Contributor Author

heavelock commented Jul 1, 2020 •
edited

Okay, flake8 fixed almost completely.

However, there are two errors that pop out:

obspy/io/xseed/blockette/blockette048.py:57:21: F507 '...' % ... has 4 placeholder(s) but 3 substitution(s)
obspy/io/xseed/blockette/blockette058.py:90:21: F507 '...' % ... has 4 placeholder(s) but 3 substitution(s)

Well the errors are there, however, last time it was touched was in 2016 by @krischer.

@heavelock
Copy link
Contributor Author

Oooookay. Dragons live there in that code. Basically, there are only few blockette objects that are tested.

Blockette010, Blockette051, Blockette053, Blockette054
Blockette041, Blockette050, Blockette054, Blockette060

So those ones are not tested at all it seems.

@heavelock
Copy link
Contributor Author

And the error came from Flake8 including support to pyflakes 2.2.x. PyCQA/flake8@76eecca

QuLogic
QuLogic reviewed Jul 2, 2020
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

The default reltol is 1, so you should just remove any explicit usage of it when it's also 1.

obspy/core/util/testing.py Outdated
@@ -337,30 +332,7 @@ def __init__(self, image_path, image_name, reltol=1,
# has been fixed.
#
# Thus test images should accurate for matplotlib >= 2.0.1 anf
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Thus test images should accurate for matplotlib >= 2.0.1 anf
# Thus test images should accurate for matplotlib >= 2.0.1.

obspy/imaging/tests/test_ppsd.py
@@ -43,11 +43,7 @@ def test_ppsd_plot_frequency(self):
"""
Test plot of ppsd example data, normal (non-cumulative) style.
"""
# mpl < 2.2 has slightly offset ticks/ticklabels, so needs a higher
# tolerance (see e.g. http://tests.obspy.org/102260)
reltol = 2
Copy link
Member

Choose a reason for hiding this comment

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

Inline this in the ImageComparison call.

obspy/signal/tests/test_spectral_estimation.py Show resolved Hide resolved
@megies
Copy link
Member

megies commented Jul 2, 2020

However, there are two errors that pop out:

obspy/io/xseed/blockette/blockette048.py:57:21: F507 '...' % ... has 4 placeholder(s) but 3 substitution(s)
obspy/io/xseed/blockette/blockette058.py:90:21: F507 '...' % ... has 4 placeholder(s) but 3 substitution(s)

I've mentioned those before in #2631, they are unrelated but should get fixed. I was hoping somebody that actually worked on the blockette stuff more could have a look, because I would have to look up details for these blockettes first and would probably just go by some examples I find.

@barsch
Copy link
Member

barsch commented Jul 2, 2020

However, there are two errors that pop out:

obspy/io/xseed/blockette/blockette048.py:57:21: F507 '...' % ... has 4 placeholder(s) but 3 substitution(s)
obspy/io/xseed/blockette/blockette058.py:90:21: F507 '...' % ... has 4 placeholder(s) but 3 substitution(s)

I've mentioned those before in #2631, they are unrelated but should get fixed. I was hoping somebody that actually worked on the blockette stuff more could have a look, because I would have to look up details for these blockettes first and would probably just go by some examples I find.

I'll take a look

@barsch barsch mentioned this pull request Jul 2, 2020
Merged
@heavelock
Copy link
Contributor Author

@QuLogic done, take a look please.

@QuLogic
Copy link
Member

QuLogic commented Jul 7, 2020

The default reltol is 1, so you should just remove any explicit usage of it when it's also 1.

Don't forget this part.

@heavelock
Copy link
Contributor Author

Fixed. @QuLogic

@heavelock
Removes if clause for matplotlib <2.0
ff150c7 
Signed-off-by: Damian Kula <dkula@unistra.fr>
@heavelock
Removes further matplotlib version checks
b44b8e2 
Signed-off-by: Damian Kula <dkula@unistra.fr>
@heavelock
Removes compatibility checks with matplotlib <3
b6d09c0 
Signed-off-by: Damian Kula <dkula@unistra.fr>
@heavelock
Removes unnecessary MATPLOTLIB_VERSION imports
eb060e4
@heavelock
Fixes imports
27a20d1 
Signed-off-by: Damian Kula <dkula@unistra.fr>
@heavelock
Addresses review comments
cd19f3a 
Signed-off-by: Damian Kula <dkula@unistra.fr>
@heavelock
Removes usages of reltol argument if it's not changed from 1 which is…
62dc623 
… default

Signed-off-by: Damian Kula <dkula@unistra.fr>
@heavelock
Copy link
Contributor Author

Rebased on current master and force pushed.

@megies
Copy link
Member

megies commented Jul 10, 2020 •
edited

Hmm.. I guess we might want to consider updating all test baseline images on master too? Since we bumped matplotlib massively and I think many of those were done on older matplotlib versions.. I guess we want to use matplotlib 3.2.0 as our new min dependency version for all baseline images from now on?

We can also do this in another PR, though. Otherwise this looks ready to go.

@heavelock
Copy link
Contributor Author

Well, you are right about that. Since now we don't have that much of a wiggle with versions, we could just update everything. Also, I think the basemap is not compatible with matplotlib 3 so we should also do that long overdue migration to cartopy.

But I think it should belong to a different PR.

@megies
Copy link
Member

megies commented Jul 10, 2020

Sounds good, lets merge this first of all then.

@megies megies merged commit 22a7c9a into obspy:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@barsch barsch barsch approved these changes

@megies megies megies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@heavelock @megies @barsch @QuLogic