Skip to content
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.

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

Add type hints for ecosystem #832

Merged
merged 7 commits into from
Feb 3, 2017

Conversation

janga1997
Copy link
Member

Fixes part of #808
I have no idea what type of a variable fitness is. Could someone help out?

@drvinceknight
Copy link
Member

Hi @janga1997, thanks for working on this.

I have no idea what type of a variable fitness is. Could someone help out?

The type is types.FunctionType. Let me know if I can assist more.

@janga1997
Copy link
Member Author

@drvinceknight In this case, can we go along with types.LambdaType?
And what is results? I assume it is an instance? (types.InstanceType)

@drvinceknight
Copy link
Member

drvinceknight commented Jan 29, 2017

@drvinceknight In this case, can we go along with types.LambdaType?

types.FunctionType is better as it allows users to pass functions that were not defined as lambda functions.

And what is results? I assume it is an instance? (types.InstanceType)

In [14]: >>> import axelrod as axl
    ...: >>> players = (axl.Cooperator(), axl.Alternator(), axl.TitForTat())
    ...: >>> tournament = axl.Tournament(players)
    ...: >>> results = tournament.play()
    ...: 
                                                                                                                                                      
In [15]: type(results)
Out[15]: axelrod.result_set.ResultSetFromFile

Although ideally the type of results should be axelrod.result_set.ResultSet. ResultSetFromFile inherits from ResultSet. However:

In [18]: type(results) is axl.result_set.ResultSet
Out[18]: False

In [19]: type(results) is axl.result_set.ResultSetFromFile
Out[19]: True

So that might need something...

@janga1997
Copy link
Member Author

@drvinceknight I kinda understand what the problem is, but I don't know the solution to it.
Should I keep it as axl.result_set.ResultSetFromFile for now?

@drvinceknight
Copy link
Member

@drvinceknight I kinda understand what the problem is, but I don't know the solution to it.

I'm not entirely sure what the solution is either (@marcharper, @Nikoleta-v3 ?).

Should I keep it as axl.result_set.ResultSetFromFile for now?

I don't see why not.

@janga1997
Copy link
Member Author

@drvinceknight I facing some trouble with the type of results? Would you mind?
Should I do an import axelrod first?

@drvinceknight
Copy link
Member

Should I do an import axelrod first?

Yup, you'll need to do that. :)

@janga1997
Copy link
Member Author

@drvinceknight Is it okay for a merge now?

@drvinceknight
Copy link
Member

@drvinceknight Is it okay for a merge now?

It looks good to me. We have a two core reviewer policy on all PRs so someone else will be around to make sure I haven't missed anything (or if they see the way to get around the ResultSet/ResultSetFromFile issue :)). 👍

@marcharper
Copy link
Member

Is it always a results set from a file?

@janga1997
Copy link
Member Author

@marcharper not sure. I faced this issue when adding type hints for derived classes of Player such as TitforTat, etc too.
We need a way to check if the object is an instance of the parent class ,and not the derived. How, I can't figure out.

@marcharper
Copy link
Member

According to the docs it looks like a subclass will type check as valid if a parent class is expected. Let's test for the parent type if multiple possible derived types are possible.

@janga1997
Copy link
Member Author

janga1997 commented Jan 31, 2017

@marcharper just to be sure, I use ResultSet for results, and Player for any kind of player, and so on?

@marcharper
Copy link
Member

marcharper commented Jan 31, 2017

Unless you know for sure that something more specific is appropriate. Also please try to run mypy on the files you annotate:
mypy --ignore-missing-imports --follow-imports skip axelrod/actions.py

You may get some errors until #837 is merged (you can rebase if you know how to do that).

@marcharper
Copy link
Member

Type checker says:


axelrod/ecosystem.py:10: error: The return type of "__init__" must be None
axelrod/ecosystem.py:40: error: Incompatible types in assignment (expression has type Callable[[Any], Any], variable has type "MethodType")
axelrod/ecosystem.py:61: error: Unsupported operand types for + ("int" and "float")
axelrod/ecosystem.py:61: error: No overload variant of "__setitem__" of "list" matches argument types [builtins.int*, builtins.float]

Let us know if you need help (and feel free to join the gitter chat to ask questions).

@janga1997
Copy link
Member Author

@marcharper The remaining errors seem to be from this line payoffs[ip] += p * pops[jp].
But I guess python typecasts int to float anyway? Is it a serious issue?

@marcharper
Copy link
Member

Try a float -- there's no reason to force an integer here -- and see if it works. Otherwise let's use Union[float, int] or some kind of generic numeric type (if that's a thing in Python 3.5).

@janga1997
Copy link
Member Author

@drvinceknight There is still one error remaining,
axelrod/ecosystem.py:40: error: Incompatible types in assignment (expression has type Callable[[Any], Any], variable has type "MethodType")

for the line
self.fitness = lambda p: p. We assigned FunctionType to fitness

@drvinceknight
Copy link
Member

@drvinceknight There is still one error remaining,
axelrod/ecosystem.py:40: error: Incompatible types in assignment (expression has type Callable[[Any], Any], variable has type "MethodType")

for the line
self.fitness = lambda p: p. We assigned FunctionType to fitness

It looks like a Callable type will do the trick:

4 from types import Callable 
...
10     def __init__(self, results: ResultSet, fitness: Callable, ...

@janga1997
Copy link
Member Author

janga1997 commented Feb 1, 2017

@drvinceknight Yeah, but didn't we decide upon FunctionType? Why are we doing Callable now?
I don't get it.

@drvinceknight
Copy link
Member

@drvinceknight Yeah, but didn't we decide upon FunctionType? Why are we doing Callable now?

I think my original advice on the use of FunctionType was too restrictive. Here's more information about the Callable type here: https://docs.python.org/3/library/typing.html#typing.Callable

https://docs.python.org/3/library/typing.html#typing.Callable

An in fact I think it should actually be:

 def __init__(self, results: ResultSet, fitness: Callable[[float], float], ...

which indicates that it's a callable that takes a float and returns a float.

@drvinceknight
Copy link
Member

Apologies, it should be:

4 from typing import Callable 

@drvinceknight
Copy link
Member

This looks fine to me now. 👍

@marcharper marcharper merged commit 7f144b2 into Axelrod-Python:master Feb 3, 2017
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.

None yet

3 participants