feature: typing annotation

[mrava87]

This PR handles https://github.com/PyLops/pylops/issues/278 and provides typing annotation to all functions and classes of pylops.

[mrava87]

@cako I noticed when adding typing annotations that the documentation becomes very ugly. Just asking in case you know a workaround before I start looking for it.

Ideally in the doc (I know it may be not be possible) I would like the function signature to look as before, as we have all the 'type hints' already below

[mrava87]

Alright, I found it (autodoc_typehints = "none")

[cako]

Let me know if you need any further help with this

[mrava87]

It should be easy for me to finish this.

One thing I was struggling a bit is to annotate inputs/outputs that are of pylops.LinearOperator type. I found some suggestions online that you can simply put the name of my class, but mypy doesn’t seem very happy about this. Any suggestion?

[mrava87]

@cako can you let me know if you have any comment/suggestion here? Otherwise I would like to merge this and proceed with the remaining modules :)

[mrava87]

Thanks!

@mrava87 thanks for the hard work, this probably took a good while!

I left a few changes through the files, but I think some general comments are in order.

• Whenever you return a, b, the type annotation should be Tuple[typeof(a), typeof(b)] as opposed to Union[...]. The case for union would be return a if something else b.

Thanks, I kept messing this up. Should be fixed everywhere now

• Whenever you have a function argument myint = None, you should annotate it with myint: Optional[int] = None. You skip the Optional only when you supply a default non-None value like myint: int = 1.

Oh, this one I didn't know. I tried to fix it everywhere (but I may have missed one or two ;) )

• Many times you used Union[Tuple, List, npt.Arraylike]. I wonder if it's not better to use a standard library base class like collections.abc.Sized or collections.abc.Collection

Mmmh you mean vp1: Union[List, Tuple, npt.ArrayLike], -> vp1: Collection (with from collections.abc import Collection?

• Why are there no annotations for LinearOperator?

I was struggling with this. Somewhere I read I should do Type[LinearOperator]: https://stackoverflow.com/questions/44664040/type-hints-with-user-defined-classes but mypy wasn't happy about it. Let's for example take Block.py. If I do ops: List[LinearOperator] or ops: List[Type[LinearOperator]], in both cases mypy gives:

pylops/basicoperators/Block.py:19: error: Module "pylops.LinearOperator" is not valid as a type
Found 1 error in 1 file (checked 1 source file)

I am going to wait on this until I hear from you. Any suggestion?

• Would it be possible to run the tests through mypy? I'm not sure if this is a thing. Basically I just want to make sure that the typing is respecting how we normally call these functions.

Technically I think it is possible, but so far I have so many errors that I am not sure it makes sense. I think one of the reasons is that we don't add typing annotation for every variable defined in the method - I find this a bit too extreme... - Unless I do ignore_errors = True which suppresses all non-fatal errors, but I fear this is a bit too aggressive... it would be good if you can take a look and see what you think.

[cako]

Give me a little longer to try to come up with a reasonable solution to the LinearOperator issue. The issue stems from the clash between the file name (a "module" in the eyes of mypy) and the class. For example, LinearOperator.py within Python doubles as the "module" LinearOperator (from . import LinearOperator for example) but also the class (from .LinearOperator import LinearOperator). This happens not only with LinearOperator but with almost every operator class. The only way I've found to get rid of this problem for good is to rename the file LinearOperator.py to something else, e.g., linear_operator.py. This is actually what is suggested by PEP8 so I don't think it's bad design choice. For the user, this change should be transparent, so I think we should take advantage of the major version and just do it. I have started doing this for all files, but haven't finished yet.

Another issue I have been facing is the clash between multiple imports. This happens because of wildcard imports used in the __init__s. For example, if we do

__all__ =  ["Convolve1D", "Smoothing1D"]
from .Convolve1D import *
from .Smoothing1D import *

but then within Smoothing1D.py we have a from .Convolve1D import Convolve1D, we now have a double import. If one enables errors in MyPy, this generates a lot of errors. The solution for this is to add __all__ = ["Smoothing1D"] in Smoothing.py (and respectively for all files). This should also have the benefit of not polluting the namespaces with spurious things, so I am also working on getting these changes implemented.

The last issue the use of npt.ArrayLike. I've actually found that this is a pretty poor type for what we want to do. It doesn't have indexing for example. Basically npt.ArrayLike only guarantees that np.asarray(x) won't fail. So whenever we use it, we should probably have an ncp.asarray converting the input. For outputs, we should avoid it, favoring something like Union[np.ndarray, cp.ndarray]. I've started working on this, but again, not done yet.

So give me a couple days and I'll circle back!

[mrava87]

Alright I see. Sounds like a good plan :)

Let me suggest something. I am done with typing annotation of signalprocessing, let me make a PR so that we don't work on both at the same time. We could merge those two PRs and then you can work on file renaming before we go ahead with the remaining modules. Sounds good?

[cako]

Sure. Let's merge these two and then I'll work on the renaming. Otherwise it's going to get very complicated!