MBravoS / splotch

Simple PLOTs, Contours and Histograms is a small package with wrapper functions designed to simplify plotting calls from matplotlib.
BSD 3-Clause "New" or "Revised" License
5 stars 0 forks source link

Deprecation of collections.Iterable #70

Closed MBravoS closed 1 year ago

MBravoS commented 1 year ago

I'm not sure how we missed this, but since Python 3.3 there has been a deprecation warning for the use of collections.Iterable (and all other aliases for classes in collections.abc), which we use in plots_1d.py. This was finally deprecated in Python 3.10, so we need to fix this, which I've done current commit in the devel branch. I'm uncertain if collections.Iterable predates collections.abc.Iterable, just in case I've added a requirement for Python 3.3+ in setup.py. This change should be fine anyway, given that v3.3 is 10 years old (!). Could you test this change @AstroRobin? If it works on your end I think it should be safe to push to master.

AstroRobin commented 1 year ago

Looks like the only function using Iterable was the curve() function, which I've ran through my own testing script for curves and all working fine, so can be pushed up to main branch.

Note, generally when I check for iterables I use "duck typing", like this:

try:  # Test whether the parameter is iterable
     _ = (k for k in val)
except TypeError:  # not iterable

I wonder which is the preferred method here and whether we should standardise it to one. Obviously using isinstance(val, Iterable) is a bit neater, but not sure if the two methods perhaps produce different outcomes in edge cases?

MBravoS commented 1 year ago

That's an interesting question. From what I found it seems that the Pythonic way is to duck-type, which seems somewhat surprising to me (I agree that using isinstance looks neater). My only concern is that strings would pass a try test for iterability, so if there's any case where we want to exclude them I think doing

if isinstance(val,Iterable) and type(val)!=str:

is better/cleaner than doing

try:
    if type(val)==str
        raise TypeError('The value is not iterable')
    _ = [v for v in val]
except TypeError:
    raise TypeError('The value is not iterable')

It's worth having a look into what is needed in each case, but I think that can be a matter for a separate release, I'll merge this bugfix in the meanwhile, close this issue and open a new one to remind us to check the code.

AstroRobin commented 1 year ago

I agree, the isinstance check seems cleaner overall. Note, that for whatever reason using checks involving comparisons to type(val) is not recommended by pep-8 (not sure why), so probably the above should look like:

if isinstance(val, Iterable) and not isinstance(val, str):

... still neater than duck-typing