Skip to content

Conversation

@bjlittle
Copy link
Member

@bjlittle bjlittle commented Aug 9, 2019

This PR fixes the iris travis-ci tests after moving to the latest numpy 1.17.0 - and it also follows-up from #3320 to dress the naked try...excepts with an ImportError

@bjlittle bjlittle force-pushed the dress-naked-excepts branch from ec4f98a to 788b967 Compare August 9, 2019 10:54
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

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

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.

@bjlittle
Copy link
Member Author

bjlittle commented Aug 9, 2019

@rcomer Fancy taking this on?

@bjlittle
Copy link
Member Author

bjlittle commented Aug 9, 2019

Closes #3365

@rcomer rcomer self-assigned this Aug 14, 2019
Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

Hi @bjlittle, I just have one minor query on the parameter checking that you are welcome to agree or disagree with. Otherwise it looks good to me.

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

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 😆.

@pp-mo
Copy link
Member

pp-mo commented Aug 20, 2019

Big Note:

We want this fixed ASAP now. As @bjlittle is currently unavailable, I'm going to adopt this + replace with my own branch + PR.
I hope you can relate to that and ideally review+merge it for us @rcomer ?? 🙏

@rcomer
Copy link
Member

rcomer commented Aug 20, 2019

I hope you can relate to that and ideally review+merge it for us

Sounds reasonable to me.

@pp-mo pp-mo mentioned this pull request Aug 20, 2019
@pp-mo
Copy link
Member

pp-mo commented Aug 20, 2019

Closed in favour of #3369

@pp-mo pp-mo closed this Aug 20, 2019
@bjlittle bjlittle deleted the dress-naked-excepts branch September 12, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants