Skip to content

Dress naked excepts #3366

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

Closed
wants to merge 3 commits into from
Closed

Dress naked excepts #3366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a23b66a
dress naked excepts
bjlittle Aug 2, 2019
f1d808c
fix tests
bjlittle Aug 9, 2019
788b967
keep sticker happy
bjlittle Aug 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/iris/_constraints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

try: # Python 3
from collections.abc import Iterable, Mapping
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Iterable, Mapping
import operator

Expand Down
2 changes: 1 addition & 1 deletion lib/iris/analysis/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
from collections import OrderedDict
try: # Python 3
from collections.abc import Iterable
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Iterable
from functools import wraps

Expand Down
19 changes: 13 additions & 6 deletions lib/iris/analysis/maths.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# (C) British Crown Copyright 2010 - 2017, Met Office
# (C) British Crown Copyright 2010 - 2019, Met Office
#
# This file is part of Iris.
#
Expand All @@ -22,10 +22,11 @@
from __future__ import (absolute_import, division, print_function)
from six.moves import (filter, input, map, range, zip) # noqa

import warnings
import inspect
import math
import operator
import inspect
import six
import warnings

import cf_units
import numpy as np
Expand Down Expand Up @@ -917,9 +918,15 @@ def ws_units_func(u_cube, v_cube):
if hasattr(data_func, 'nin'):
self.nin = data_func.nin
else:
(args, varargs, keywords, defaults) = inspect.getargspec(data_func)
self.nin = len(args) - (
len(defaults) if defaults is not None else 0)
if six.PY2:
(args, _, _, defaults) = inspect.getargspec(data_func)
self.nin = len(args) - (
len(defaults) if defaults is not None else 0)
else:
sig = inspect.signature(data_func)
args = [param for param in sig.parameters.values()
if '=' not in str(param)]
self.nin = len(args)
Copy link
Member Author

@bjlittle bjlittle Aug 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Avoid DeprecationWarning in Python3:

DeprecationWarning: inspect.getargspec() is deprecated since Python 3.0, use inspect.signature() or inspect.getfullargspec()

Also, signature of numpy.cumsum from 1.17.0 has changed resulting in inspect.getargspec not working here. Hence the move to inspect.signature in order to get iris.tests.test_basic_maths.TestIFunc.test_ifunc to pass again.

Copy link
Member

Choose a reason for hiding this comment

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

I'm new to inspect.Signature and inspect.Parameter, but would this also work?

args = [param for param in sig.parameters.values() if param.default is param.empty]

It seems a bit more direct than searching the string.

Not really important, but I figure I ought to find something to comment on 😆.

Copy link
Member

Choose a reason for hiding this comment

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

I'm new to inspect.Signature and inspect.Parameter,

Me too, and it's more complicated than I expected...

Digging a little deeper, I think the above doesn't quite work, because (I found), since Python 3 now supports keyword-only parameters, it is now possible to have a keyword without a default : It becomes a required keyword.
Like this :

>>> def myfn(a, b, *, reqd_key):
...   return (a, b, reqd_key)
... 
>>> myfn(1,2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: myfn() missing 1 required keyword-only argument: 'reqd_key'
>>> myfn(1,2,3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: myfn() takes 2 positional arguments but 3 were given
>>> myfn(1,2, reqd_key=3)
(1, 2, 3)

>>> sig = inspect.Signature.from_callable(myfn)
>>> for name, par in sig.parameters.items():
...   print("'{}': kind={}, default={}".format(name, par.kind, par.default))
... 
'a': kind=1, default=<class 'inspect._empty'>
'b': kind=1, default=<class 'inspect._empty'>
'reqd_key': kind=3, default=<class 'inspect._empty'>
>>>

Worse (?!?!), Python 3 since (I think) 3.3 now supports the concept of positional-only parameters
- even though a Python syntax for defining them only exists from Python 3.8 (I am really not making this up).

So to be safe, I think we now need to use something like

    [param for param in sig.parameters.values()
     if param.kind in (Parameter.POSITIONAL_ONLY, Parameter.POSITIONAL_OR_KEYWORD)]

Urrgh.
Anyone for going back to small+simple Python2 ??

Copy link
Member

Choose a reason for hiding this comment

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

POSITTIONAL_OR_KEYWORD includes oldschool keywords, e.g. axis in np.cumsum, that we specifically don't want to count:

In [3]: sig = inspect.Signature.from_callable(np.cumsum)                                                                                                      

In [4]: for name, par in sig.parameters.items(): 
   ...:     print("'{}': kind={}, default={}".format(name, par.kind, par.default)) 
   ...:                                      

'a': kind=1, default=<class 'inspect._empty'>
'axis': kind=1, default=None
'dtype': kind=1, default=None
'out': kind=1, default=None

So I think we still want to check the defaults. Maybe

if param.kind != param.KEYWORD_ONLY and param.default is param.empty

?

In any case, we are still relying on numpy type functions sticking to a convention that arrays are "positional" parameters and all other parameters are "keywords". This is possibly not ideal, though seems to have served us OK so far.

Copy link
Member

@pp-mo pp-mo Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think you are exactly right @rcomer. (and I was wrong!)

Any "normal, old-fashioned named parameter" could be required (== has no default) or optional (== has a default), but inspect still labels all those as 'POSITIONAL_OR_KEYWORD', so this "includes oldschool keywords, e.g. axis in np.cumsum" as you said.

We want to count the "required positional arguments", which means ignoring anything optional, i.e. with a default. The only change is, since Python3, we now have keyword-onlys, which may have no default but must also be excluded from the count.


if self.nin not in [1, 2]:
msg = ('{} requires {} input data arrays, the IFunc class '
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/coords.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
from collections import namedtuple
try: # Python 3
from collections.abc import Iterator
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Iterator
import copy
from itertools import chain
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/cube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
Mapping,
MutableMapping,
Iterator)
except: # Python 2.7
except ImportError: # Python 2.7
from collections import (Iterable,
Container,
Mapping,
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/fileformats/cf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@

try: # Python 3
from collections.abc import Iterable, MutableMapping
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Iterable, MutableMapping
import os
import re
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/io/format_picker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@

try: # Python 3
from collections.abc import Callable
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Callable
import functools
import os
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/iterate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

try: # Python 3
from collections.abc import Iterator
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Iterator
import itertools
import warnings
Expand Down
7 changes: 3 additions & 4 deletions lib/iris/tests/test_basic_maths.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,14 +596,13 @@ def vec_mag(u, v):
def vec_mag_data_func(u_data, v_data):
return np.sqrt( u_data**2 + v_data**2 )

vec_mag_ifunc = iris.analysis.maths.IFunc(vec_mag_data_func, lambda a,b: (a + b).units)
vec_mag_ifunc = iris.analysis.maths.IFunc(vec_mag_data_func,
lambda a, b: (a + b).units)
b2 = vec_mag_ifunc(a, c)

self.assertArrayAlmostEqual(b.data, b2.data)

cs_ifunc = iris.analysis.maths.IFunc(np.cumsum,
lambda a: a.units
)
cs_ifunc = iris.analysis.maths.IFunc(np.cumsum, lambda a: a.units)

b = cs_ifunc(a, axis=1)
ans = a.data.copy()
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/tests/test_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

try: # Python 3
from collections.abc import Iterable
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Iterable
import datetime
import itertools
Expand Down
2 changes: 1 addition & 1 deletion lib/iris/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

try: # Python 3
from collections.abc import Hashable
except: # Python 2.7
except ImportError: # Python 2.7
from collections import Hashable
import abc
from contextlib import contextmanager
Expand Down