Add type checks

[mschmidt87]

This PR adds type checks to the library and implements changes requested by the static type checker mypy. Eventually, it will also add mypy to the CI.

Note: This is a draft PR and was created now to raise awareness of this. To run mypy, execute:

pip install mypy
cd gp/
mypy *.py

This will currently raise a bunch of errors that need to be addressed before this PR can get merged. Also, the current state of the code introduces a circular import which will make importing the library fail.

[jakobj]

I've added a few comments, some of the code seems to be outdated, e.g., the use of prefixes or abstract classes. can you specify where exactly problems occur? for example, what makes the circular import necessary?

[mschmidt87]

Thank you for your comments, I fixed most of them, the most important point and obstacle towards getting mypy passing is the problem with the imports, which is still mysterious to me.

[jakobj]

thanks for the update! let's discuss the remaining issues per video.

[mschmidt87]

After getting a reply in the mypy-issue, I now fixed all the imports to be relative instead of absolute.
Note that with this PR, the library is currently not usable due to some cyclic imports. My plan is to first make mypy pass and then deal with that. We will likely need to introduce some stub type definitions to avoid the cyclic imports.

To run mypy, do the following:

cd gp
cd ..
mypy gp

I am currently getting these errors:

gp/genome.py:127: error: List item 0 has incompatible type "None"; expected "int"
gp/genome.py:128: error: Incompatible return value type (got "List[int]", expected "List[Optional[int]]")
gp/genome.py:128: note: "List" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance
gp/genome.py:128: note: Consider using "Sequence" instead, which is covariant
gp/genome.py:128: note: Perhaps you need a type annotation for "region"? Suggestion: "List[Optional[int]]"
gp/genome.py:150: error: List item 0 has incompatible type "None"; expected "int"
gp/genome.py:170: error: Argument 1 to "__iadd__" of "list" has incompatible type "List[Optional[int]]"; expected "Iterable[int]"
gp/genome.py:431: error: Argument 6 to "Genome" has incompatible type "Primitives"; expected "List[Type[Node]]"
gp/cartesian_graph.py:12: error: Incompatible types in assignment (expression has type "None", variable has type Module)
gp/cartesian_graph.py:109: error: Argument 2 to "__call__" of "Node" has incompatible type "List[int]"; expected "CartesianGraph"
gp/cartesian_graph.py:161: error: Argument 1 to "_hidden_column_idx" of "CartesianGraph" has incompatible type "Optional[int]"; expected "int"
Found 8 errors in 2 files (checked 12 source files)

There are three main problems that need to be resolved:

• This error: gp/genome.py:431: error: Argument 6 to "Genome" has incompatible type "Primitives"; expected "List[Type[Node]]" is because the clone() function of the Genome class puts its _primitives members as an input variable to its __init__ function to generate the cloned class. However, these types are incompatible since _primitives is an instance of Primitives, but in the __init__ functions, the instance of Primitives is generated from the input variable, thus this is somewhat circular. This should also fail at the moment, as far as I can see.
• mypy doesn't like the construction about checking the availability of torch and sympy. I think the best way to solve this is to rather use a torch_found boolean variable and check for this than to directly check if torch.
• mypy complains about the use of lists for indexing regions in the genome. The reason is that we use either integers of None and mypy demands lists to be invariant, i.e., to contain only one type. See this documentation for some explanation: https://mypy.readthedocs.io/en/latest/common_issues.html#invariance-vs-covariance
  Thus, we can either use something else than None for non-used genes (e.g. -1) or we refrain from using lists and replace them by an immutable object like e.g. a tuple.

[mschmidt87]

One more point:
The __call__ method of the Node class has this signature:
https://github.com/Happy-Algorithms-League/python-gp/blob/c98cf32ca8b645c35ae6a6b74a370b19efef4456/gp/node.py#L168
However, the x appears to be unused in all child class of Node. Can it be removed?
Also, this __call__ method sets the output_ member of the Node class and seems to be used only in the CartesianGraph.parse_genome() method:
https://github.com/Happy-Algorithms-League/python-gp/blob/c98cf32ca8b645c35ae6a6b74a370b19efef4456/gp/cartesian_graph.py#L108

and in the __call__ method of the CartesianGraph.
The more relevant member for actually computing the function of the graph is _output_str if I'm not mistaken.
@jakobj

[jakobj]

This error: gp/genome.py:431: error: Argument 6 to "Genome" has incompatible type "Primitives"; expected "List[Type[Node]]" is because the clone() function of the Genome class puts its _primitives members as an input variable to its __init__ function to generate the cloned class. However, these types are incompatible since _primitives is an instance of Primitives, but in the __init__ functions, the instance of Primitives is generated from the input variable, thus this is somewhat circular. This should also fail at the moment, as far as I can see.

how about supporting the list() constructor so we can call list(self._primitives) in Genome::clone?

mypy doesn't like the construction about checking the availability of torch and sympy. I think the best way to solve this is to rather use a torch_found boolean variable and check for this than to directly check if torch.

I find it less pretty than the current implementation that does not introduce an additional variable, but if mypy is unhappy otherwise, feel free to change it. i guess a comment would help there.

mypy complains about the use of lists for indexing regions in the genome. The reason is that we use either integers of None and mypy demands lists to be invariant, i.e., to contain only one type. See this documentation for some explanation: https://mypy.readthedocs.io/en/latest/common_issues.html#invariance-vs-covariance
Thus, we can either use something else than None for non-used genes (e.g. -1) or we refrain from using lists and replace them by an immutable object like e.g. a tuple.

we can use -3 for unused genes in the InpuNode/OutputNode sections. we've already introduced -1 and -2 as special values so why not an additional one ;)

[jakobj]

One more point:
The __call__ method of the Node class has this signature:
[...snip...]

unfortunately i don't remember why we need the x there. input values seem to be retrieved by accessing input nodes via the graph instance. in the beginning we provided the option of calling the graph directly without calling any to...method. i am now wondering how input values are passed into the graph if x is never used? we can either add tests to make sure this functionality works as expected, or we force the user to always call one of the to... functions. i am inclined to follow the latter path, just because it reduces complexity without any significant drawback (that i'm currently aware of).

note that __call__ is not used in https://github.com/Happy-Algorithms-League/python-gp/blob/c98cf32ca8b645c35ae6a6b74a370b19efef4456/gp/cartesian_graph.py#L108 as primitives does not store instances, but classes, so this calls the constructor, not the __call__ function!

[jakobj]

awesome, thanks for the hard work! 🎉 🎉 i've reviewed until genome.py, will continue there later, here are already a few comments.

[jakobj]

getting better every time! ✨ added a few comments that should be addressed before merging.

[coveralls]

Coverage increased (+0.02%) to 97.315% when pulling f07f926 on add_type_checks into a149430 on master.

[mschmidt87]

I addressed all your comments, @jakobj . Please take another look.

[jakobj]

appreciate the hard work 🙇 only some superficial comments left

[jakobj]

awesome work! 🎉 ✨ 🎉 so 👍 conditional on travis passing