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

PPSD: Need to adjust to set_clim API change in matplotlib 3.1-3.3 #2652

Merged
merged 1 commit into from Jun 29, 2020

Conversation

megies
Copy link
Member

@megies megies commented Jun 26, 2020

We need to adjust some PPSD plotting internals to a matplotlib API change:

[...]\obspy\signal\spectral_estimation.py:2096: MatplotlibDeprecationWarning: 
The set_clim function was deprecated in Matplotlib 3.1 and will be removed in 3.3. Use ScalarMappable.set_clim instead.
  cb.set_clim(*fig.ppsd.color_limits)

@millipedes this is something pretty simple and confined, if you wanna give it a shot? It might need an if/else depending on matplotlib version, but actually my guess is that the mentioned ScalarMappable.set_clim should be in all old matplotlibs as well, but needs to be checked. This should be worked on branching off of maintenance_1.2.x

@megies megies added task .imaging Issues with our plotting functionalities priority: high labels Jun 25, 2020
@megies megies added this to the 1.2.2 milestone Jun 25, 2020
@megies
Copy link
Member Author

megies commented Jun 25, 2020

1.2.x is still very lax about matplotlib version, so we should make sure the to-be-changed matplotlib calls work on old versions:

obspy/setup.py

Line 113 in 2926827

'matplotlib>=1.1.0',

@krischer
Copy link
Member

Do you want to do this for 1.2.2? Might be a good idea but also not super urgent.

@megies
Copy link
Member Author

megies commented Jun 25, 2020

Ideally yes, since we never know if anything upcoming will ever warrant a 1.2.3

@millipedes
Copy link

@megies I will start to take a look and post any questions I may have. Thank you. I have also been looking into issue #2605 and will likely be posting some questions/comments there a little later today.

@millipedes
Copy link

@megies, here is my understanding of the issue, please let me know if I have erred (branching off of maintenance_1.2.x):

  1. First determine if ScalarMappable.set_clim is in older versions of matplotlib
  2. if so then change obspy/signal/spectral_estimation.py to use it
  3. if not then make an if/else statement that would either have the user using set_clim or ScalarMappable.set_clim depending on version.

I am a little confused on the matplotlib version. At the moment I see that the current master branch has 'matplotlib>=3.2.0', but I believe that we are looking to support older versions of obspy? If so, how can I access older versions?

CC
@dsentinel

@megies
Copy link
Member Author

megies commented Jun 26, 2020

@millipedes yes, thats exactly what is to do here.

At the moment I see that the current master branch has 'matplotlib>=3.2.0'

The master branch is the base for an upcoming obspy 2.0 release with massive changes, like dropping Python 2 support and all the compatibility gymnastics we been doing for that. we also bumped dependency versions massively.

The maintenance_... branches are for pure bug fix / point releases, not changing any features of the base version.

You can also have a look at this: https://github.com/obspy/obspy/wiki#developer-corner
Ans specifically the branching model: https://github.com/obspy/obspy/wiki/ObsPy-Git-Branching-Model

.., but I believe that we are looking to support older versions of obspy? If so, how can I access older versions?

This will be a fix on the maintenance_1.2.x branch, so the older dependency version still hold there.

In any case @millipedes, things have changed a bit with all the reports of installation problems with newest numpy, so we'll want to do the release ASAP, so I'll handle this one myself I think, to avoid potential back and forth communication. I'll ping you in other issues again, that don't have such a strong time constraint. :-)

@megies
Copy link
Member Author

megies commented Jun 26, 2020

Should be OK like this, let's see what CI has to say

@megies
Copy link
Member Author

megies commented Jun 29, 2020

Looks OK on those Travis builds that can still run. I think all the older ones (2.7 etc) are already switched off on Travis and Appveyor

@megies megies merged commit 60733f0 into maintenance_1.2.x Jun 29, 2020
@megies megies deleted the fix_mpl_set_clim branch June 29, 2020 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.imaging Issues with our plotting functionalities priority: high task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants