Remove dynamic classes and tidy imports #852

drvinceknight · 2017-02-05T13:46:54Z

This gets rid of the dynamic lookerup classes.

I also tided a number of things in our imports to make type checking happier:

This is the remaining output of running mypy on the full library:

Axelrod(845) ✗: mypy --ignore-missing-imports --follow-imports skip axelrod
axelrod/player.py:92: error: Need type annotation for variable
axelrod/strategies/meta.py:388: error: Invalid type "axelrod.strategies.meta.NiceMetaWinnerEnsemble"
axelrod/strategies/meta.py:388: error: Invalid base class
axelrod/strategies/meta.py:400: error: Invalid type "axelrod.strategies.meta.NiceMetaWinnerEnsemble"
axelrod/strategies/meta.py:400: error: Invalid base class
axelrod/strategies/meta.py:411: error: Invalid type "axelrod.strategies.meta.NiceMetaWinnerEnsemble"
axelrod/strategies/meta.py:411: error: Invalid base class
axelrod/strategies/meta.py:422: error: Invalid type "axelrod.strategies.meta.NiceMetaWinnerEnsemble"
axelrod/strategies/meta.py:422: error: Invalid base class
axelrod/strategies/meta.py:433: error: Invalid type "axelrod.strategies.meta.NiceMetaWinnerEnsemble"
axelrod/strategies/meta.py:433: error: Invalid base class
axelrod/__init__.py:19: error: Name 'strategies' already defined
axelrod/tests/unit/test_windows_detection.py:9: error: 'overload' decorator expected
axelrod/tests/unit/test_windows_detection.py:15: error: 'overload' decorator expected
axelrod/tests/unit/test_player.py:170: error: Need type annotation for variable
axelrod/tests/unit/test_interaction_utils.py:26: error: List item 0 has incompatible type "Tuple[Tuple[str, str], float]"
axelrod/tests/unit/test_interaction_utils.py:26: error: List item 1 has incompatible type "Tuple[Tuple[str, str], float]"
axelrod/tests/unit/test_interaction_utils.py:27: error: List item 0 has incompatible type "Tuple[Tuple[str, str], float]"
axelrod/tests/unit/test_interaction_utils.py:28: error: List item 0 has incompatible type "Tuple[Tuple[str, str], float]"
axelrod/tests/unit/test_interaction_utils.py:28: error: List item 1 has incompatible type "Tuple[Tuple[str, str], float]"

I suggest that once this PR is merged we take that list and open another issue aiming to get rid of those lasting items.

marcharper · 2017-02-05T18:19:57Z

Can this be fixed without removing strategy transformers?

axelrod/strategies/meta.py:388: error: Invalid type "axelrod.strategies.meta.NiceMetaWinnerEnsemble"
axelrod/strategies/meta.py:388: error: Invalid base class

drvinceknight · 2017-02-05T18:30:56Z

Can this be fixed without removing strategy transformers?

I have no idea.

It took me quite a while to get this far so I suggest that gets looked in to after this PR. Either way we're not going to remove strategy transformers right?

marcharper · 2017-02-05T18:26:50Z

axelrod/strategies/averagecopier.py

@@ -21,8 +22,7 @@ class AverageCopier(Player):
        'manipulates_state': False
    }

-    @staticmethod
-    def strategy(opponent: Player) -> Action:


because of the random?

This was being pointed out as an error by mypy something to do with the method not being in line with the parent class.

marcharper · 2017-02-05T18:28:24Z

axelrod/strategies/lookerup.py

-    # Add the generated class to the module dictionary so it can be
-    # imported into other modules
-    setattr(module, class_name, new_class)
+class EvolvedLookerUp0_0_1(LookerUp):


I don't know that we actually want to add all of these to the library. I only included a few in the axelrod.strategies list -- maybe just add those? Some of the small ones shadow strategies in the library already, and the ones with larger parameters don't seem to outperform 2_2_2 for the most part.

Sure, you only include 1-1-1 and 2-2-2 in the whole list. Should we go ahead and just include those?

marcharper · 2017-02-05T18:34:55Z

It took me quite a while to get this far so I suggest that gets looked in to after this PR. Either way we're not going to remove strategy transformers right?

I don't know? If not then why are we removing the dynamic class generation for LookerUp? For readability? I'm not opposed per se I'm just not sure what problem it solves.

drvinceknight · 2017-02-05T18:45:17Z

I don't know?

We certainly should not remove the strategy transformers.

If not then why are we removing the dynamic class generation for LookerUp? For readability? I'm not opposed per se I'm just not sure what problem it solves.

Readability would be a big reason for me, I'd almost turn this around the other way, going back to my original question on #845: what problem does having dynamically generated classes solve?

I actually found this quite hard to untangle working on this PR so would be strongly in favour of verbose class definitions (in line with the rest of the library). I'd also suggest that if we are only going to have 1-1-1 and 2-2-2 (which I'm fine with) then we could get rid of the data file for lookerups.

I'd actually suggest that the whole reading in of data from the external source doesn't help with readability. For example in this particular case there was an error in the header which caused me a 5 minute labyrinth run around but that's perhaps a discussion for another PR.

marcharper · 2017-02-05T19:10:22Z

Ok, I understand. The reason I did it the way it currently is was so that if someone else wants to play with the other variants they can easily, and they wouldn't be tempted to add new variants that had already been created. It was also easy to add a bunch of variants to the library by just adding new lines in the data file rather than write new classes each time, which is pretty tedious and involves a lot of repeated boilerplate that's easy to mess up -- class name, data, player name, parent class... it was much easier to just comment out a line in the data file and add a new one. The dynamic code is ~20 lines, all the individuals classes written out are ~300.

I also didn't want to remove all the variants, but I didn't want to add 15 very similar strategies to the standard tournament -- we already have a number of players that are quite similar, like the gobymajorities and retaliators.

So I don't think there is a great solution, but if we want to say readability takes precedent then I'll go along with that.

Regardless I think it's better no having large static data chunks in the strategy classes themselves, so I would prefer to keep the data loading as is.

drvinceknight · 2017-02-05T19:16:16Z

The dynamic code is ~20 lines, all the individuals classes written out are ~300.

As I said, I'm not sure it's self evident that that's a good thing.

So I don't think there is a great solution, but if we want to say readability takes precedent then I'll go along with that.

Yes my vote goes to readability. It would be a minor refactor to handle all this with inheritance which would appease your previous concern (and keep in line with the rest of the library with other such cases).

Regardless I think it's better no having large static data chunks in the strategy classes themselves, so I would prefer to keep the data loading as is.

I would personally suggest keeping things in one spot where it's not tedious to do is better with the argument of removing complexity (I went on a bit of a merrygoround ride when working on this).

I've just pushed removing the various ones that were not added to the list, so only have 2_2_2 and 1_1_1. I have removed the data loading for this case as it was in essence there for reading two short strings which felt silly.

drvinceknight · 2017-02-05T19:30:31Z

09daa29 refactors the code a bit more so that it makes use of inheritance.

drvinceknight · 2017-02-05T19:34:12Z

Regardless I think it's better no having large static data chunks in the strategy classes themselves, so I would prefer to keep the data loading as is.

I've also just pushed some changes that removed unused code (mainly relevant to reading in the two patterns). None of my main changes (including the inheritance) are incompatible with reading in the data if we really want that to stay.

marcharper · 2017-02-05T21:25:12Z

axelrod/strategies/lookerup.py



-class Winner12(LookerUp):


I prefer inheriting from LookerUp to reduce the number of inherited classes. It seems odd to me that Winner12 would derive from EvolvedLookerUp1_1_1 (same for the others that derive from it).

Perhaps we can assume that the lookup table is specified by a pattern.

I've gone with an intermediate class. Let me know what you think.

marcharper · 2017-02-05T21:25:48Z

axelrod/data/lookup_tables.csv

@@ -1,19 +0,0 @@
-# Name (string), plays (int), opp_plays(int), starting_plays(int), pattern (str)


Can we find somewhere else to save this data? (the evolver repository, perhaps)

Good call: Axelrod-Python/axelrod-dojo#20

marcharper · 2017-02-05T21:26:40Z

axelrod/strategies/lookerup.py

+    A 1_1_1 Lookerup trained with an evolutionary algorithm.
+
+    Names:
+        - Evolved Lookerup 1 1 1: Original name by Marc Harper


The names are inconsistent -- let's pick underscores or no underscores.

marcharper · 2017-02-06T15:24:25Z

I think kwarg arguments for the pattern make more sense then the intermediate class having member variables. If I wanted to instantiate a new lookerup (say a 4, 4, 2) I have to make a derived classed; with arguments I could just use the existing abstract base class.

Otherwise I'm happy with the PR.

drvinceknight · 2017-02-06T15:26:11Z

I think kwarg arguments for the pattern make more sense then the intermediate class having member variables. If I wanted to instantiate a new lookerup (say a 4, 4, 2) I have to make a derived classed; with arguments I could just use the existing abstract base class.

I'm not sure I understand. Are you talking about using kwargs in the init method?

drvinceknight · 2017-02-06T15:36:53Z

I'm not sure I understand. Are you talking about using kwargs in the init method?

I might have it, just working on a commit now :)

drvinceknight · 2017-02-06T15:50:43Z

Let me know if e7b02d6 is what you had in mind.

marcharper · 2017-02-07T03:09:30Z

Thanks, that's what I wanted :) I approved the changes, looks like we need a rebase and then we're good.

drvinceknight · 2017-02-07T18:20:55Z

Thanks, that's what I wanted :) I approved the changes, looks like we need a rebase and then we're good.

Cool, just pushed that :) I also added another test for the pattern init.

marcharper · 2017-02-09T15:36:00Z

@drvinceknight can you rebase/merge?

drvinceknight · 2017-02-09T20:15:08Z

@drvinceknight can you rebase/merge?

Just seen this :) Will have dinner and do it after.

drvinceknight · 2017-02-09T20:35:46Z

@drvinceknight can you rebase/merge?

Done. 👍

drvinceknight added the ready-for-review label Feb 5, 2017

marcharper reviewed Feb 5, 2017

View reviewed changes

drvinceknight force-pushed the 845 branch from a66dcc5 to 4971d50 Compare February 5, 2017 19:09

marcharper requested changes Feb 5, 2017

View reviewed changes

marcharper approved these changes Feb 7, 2017

View reviewed changes

drvinceknight force-pushed the 845 branch from e7b02d6 to 4291665 Compare February 7, 2017 18:20

marcharper added the ready-to-merge label Feb 8, 2017

drvinceknight added 7 commits February 9, 2017 20:31

Remove redundant code in ann.

da82186

Remove dynamic classes.

6babf1f

Add ann to type_tests.sh.

8add4ea

Fix error in lookerup.

bf4e5cf

Correct imports for type checking.

3f2e822

Remove static methods to make type checker happy.

c226cef

Only include 2 evolved lookerups.

5759388

drvinceknight added 7 commits February 9, 2017 20:33

Remove uneeded data file.

0e7df02

Refactor code to use inheritance further.

a6a663e

Remove unused code.

8add4c7

Implement intermediate class and fix names.

54dab9a

Remove intermediate class and make use of kwargs.

0488e25

Fix merge conflict error.

6b220f3

Add a test for pattern lookup.

8d6733f

drvinceknight force-pushed the 845 branch from 4291665 to 8d6733f Compare February 9, 2017 20:34

meatballs merged commit c2dba09 into master Feb 10, 2017

meatballs deleted the 845 branch February 10, 2017 09:32

This was referenced Feb 10, 2017

Add type hints to apavlov #846

Merged

Add type hints to load_data #831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dynamic classes and tidy imports #852

Remove dynamic classes and tidy imports #852

drvinceknight commented Feb 5, 2017 •

edited

Loading

marcharper commented Feb 5, 2017

drvinceknight commented Feb 5, 2017

marcharper Feb 5, 2017

drvinceknight Feb 5, 2017

marcharper Feb 5, 2017

drvinceknight Feb 5, 2017

marcharper commented Feb 5, 2017

drvinceknight commented Feb 5, 2017 •

edited

Loading

marcharper commented Feb 5, 2017

drvinceknight commented Feb 5, 2017

drvinceknight commented Feb 5, 2017 •

edited

Loading

drvinceknight commented Feb 5, 2017

marcharper Feb 5, 2017 •

edited

Loading

drvinceknight Feb 6, 2017

marcharper Feb 5, 2017

drvinceknight Feb 6, 2017

marcharper Feb 5, 2017

drvinceknight Feb 6, 2017

marcharper commented Feb 6, 2017

drvinceknight commented Feb 6, 2017

drvinceknight commented Feb 6, 2017

drvinceknight commented Feb 6, 2017

marcharper commented Feb 7, 2017

drvinceknight commented Feb 7, 2017

marcharper commented Feb 9, 2017

drvinceknight commented Feb 9, 2017

drvinceknight commented Feb 9, 2017

		@@ -1,19 +0,0 @@
		# Name (string), plays (int), opp_plays(int), starting_plays(int), pattern (str)

Remove dynamic classes and tidy imports #852

Remove dynamic classes and tidy imports #852

Conversation

drvinceknight commented Feb 5, 2017 • edited Loading

marcharper commented Feb 5, 2017

drvinceknight commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper commented Feb 5, 2017

drvinceknight commented Feb 5, 2017 • edited Loading

marcharper commented Feb 5, 2017

drvinceknight commented Feb 5, 2017

drvinceknight commented Feb 5, 2017 • edited Loading

drvinceknight commented Feb 5, 2017

marcharper Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper commented Feb 6, 2017

drvinceknight commented Feb 6, 2017

drvinceknight commented Feb 6, 2017

drvinceknight commented Feb 6, 2017

marcharper commented Feb 7, 2017

drvinceknight commented Feb 7, 2017

marcharper commented Feb 9, 2017

drvinceknight commented Feb 9, 2017

drvinceknight commented Feb 9, 2017

drvinceknight commented Feb 5, 2017 •

edited

Loading

drvinceknight commented Feb 5, 2017 •

edited

Loading

drvinceknight commented Feb 5, 2017 •

edited

Loading

marcharper Feb 5, 2017 •

edited

Loading