search

pik-copan / pyunicorn

Unified Complex Network and Recurrence Analysis Toolbox
http://pik-potsdam.de/~donges/pyunicorn/
Other
195 stars 86 forks source link

LINT: disabled `pylint` flags should be re-enabled where possible, or disabled locally #196

Open fkuehlein opened 9 months ago

fkuehlein commented 9 months ago

CI build previously failed due to pylint complaining about 

E0110: Abstract class 'PlateCarree' with abstract methods instantiated (abstract-class-instantiated)

at two instances within climate.CartopyPlots.init() (see Travis CI log).

The flag was raised in the python=3.8-build only, later python version were fine. pyunicorn's handling of cartopy is perfectly in line with cartopy docs. Also — to my understanding — PlateCarree as not actually an abstract class itself, although some parent classes are (see cartopy source). What's more, there is a similar false-positive issue open at pylint.

Therefore, I will locally disable this flag for now. I am filing this issue, because this can only be a temporary solution and should be kept track of. Also, other currently disabled pylint-flags should be looked into, or disabled only locally where it makes sense.

On another side note: Deprecation/dropping of climate.MapPlots might be looked into as well. Is its functionality fully covered by climate.CartopyPlots and cartopy/matplotlib?

ntfrgl commented 9 months ago

I think that locally disabling this warning is the correct permanent strategy here, following Pylint's general advice as well as general practice. In particular, the underlying technical problem is arbitrarily complex (the task of statically predicting the dynamic presence or absence of methods, in a programming language with dynamically typed reflection, is in general equivalent to the halting problem). In this sense, Pylint should be understood as a collection of heuristics, and while improving the heuristics would be valuable, both the currently implemented heuristic and the only alternative heuristic suggested so far (in the 4 years since the issue you linked was opened) appear to be inadequate (yielding false positive) in our scenario for some interpreter versions.

Regarding other Pylint warnings that appear only in very few places, locating them would probably be beneficial as well, since it is trivial to find pylint: comments.

On another side note: Deprecation/dropping of climate.MapPlots might be looked into as well. Is its functionality fully covered by climate.CartopyPlots and cartopy/matplotlib?

This should be discussed with @jdonges and @zugnachpankow.

fkuehlein commented 9 months ago

Sounds totally reasonable now that you put it that way, thank you for the illustration.

This should be discussed with @jdonges and @zugnachpankow.

Also agreed, will put that forth on the next occasion ..

zugnachpankow commented 9 months ago

Also agreed, will put that forth on the next occasion ..

I am available to look into this with you. Basic features should be covered though.

fkuehlein commented 9 months ago

thanks for your availability @zugnachpankow, I will look into it myself and get back to you!

fkuehlein commented 5 months ago

Working on this cleanup on the linked branch. Some more severe issues found that I will address later:

fkuehlein commented 1 day ago

Most pylint flags have been resolved as of #224. Cleaned up Flake8 in #233.

Leave the issue open for the remaining pylint flags given in #224.