Algorithm for Memory Depth in FSM

[gaffney2010]

As discussed here:
#597

[marcharper]

Maybe move these classes out of the function, it doesn't seem like they use any local data in their definitions.

[gaffney2010]

You're right that they could be moved outside of the function without problem. My reasoning for leaving them in the function is because they're not used anywhere else, and my feeling is that it's similar to a local variable, which could be made global, but encapsulation is better. I know this is a contentious point though.

[meatballs]

I'd prefer to see these classes outside the function and with unit tests for their behaviour.

[marcharper]

The travis error is as follows (docstring format):

Warning, treated as error:
/home/travis/build/Axelrod-Python/Axelrod/axelrod/strategies/finite_state_machines.py:docstring of axelrod.strategies.finite_state_machines.get_memory_from_transitions:36:Unexpected indentation.
make: *** [html] Error 2
The command "cd docs; make clean; make html" exited with 2.

The tests are also outputing all the branches as the algorithm proceeds, I'd make that optional (with a verbose kwarg):

If _/D, C/_, then D
If _/C, C/_, then C
If _/C, D/_, then D
If _/D, C/D, D/_, then D
If _/C, D/D, D/_, then D
If _/D, D/D, D/_, then C
...

[marcharper]

Can you apply the algorithm to the existing FSM players in the library? The current memory depth values were best guesses at the time the strategies were added. In some cases we inherit from the TestPlayer class to add additional tests, in this case something like TestFSMPlayer where it applies your algorithm to verify, at test time, that the player is of the expected depth.

[marcharper]

We could also consider adding a method to FSMPlayer's __init__ to compute the memory_depth if it is unspecified. This will come in handy for e.g. the dojo so we can evolve players of depth <= N.

[gaffney2010]

The travis error is as follows (docstring format):

Fixed.

The tests are also outputing all the branches as the algorithm proceeds, I'd make that optional (with a verbose kwarg):

I added a print_output flag to go along with the (even more verbose) print_trace flag.

[gaffney2010]

Can you apply the algorithm to the existing FSM players in the library?

I'll do that.

in this case something like TestFSMPlayer where it applies your algorithm to verify, at test time, that the player is of the expected depth.

Currently the runtime for large FSMs is too long to be tolerable. [Like many times longer than the rest of the tests put together.] I'll see if there are any easy speed-ups.

[drvinceknight]

Currently the runtime for large FSMs is too long to be tolerable. [Like many times longer than the rest of the tests put together.] I'll see if there are any easy speed-ups.

A suggestion: one option could be to have a separate repo where we run the algorithm on the fsms perhaps?

[gaffney2010]

These 16-state strategies take a very long time. I guess the number of branches is growing exponentially. I'm going have to get this thing to run faster. I think there's a memoization here that makes sense.

[gaffney2010]

I may end up changing stuff for the caching logic. Closing for now.

[gaffney2010]

In thinking about how to memoize, I ended up changing significantly. It brought the runtime down from days to milliseconds.

The new approach is a little abstract, and it may be harder to see that it works, but it does match the other implementation on every FSM strategy, and it passes all of the tests.

[gaffney2010]

in this case something like TestFSMPlayer where it applies your algorithm to verify, at test time, that the player is of the expected depth.

We could also consider adding a method to FSMPlayer's init to compute the memory_depth if it is unspecified. This will come in handy for e.g. the dojo so we can evolve players of depth <= N.

I can probably do these now that it runs fast.

[marcharper]

The error is on the typing syntax, which isn't supported on Python 3.5. However we generally only support the last two trailing versions of Python, so it's time we updated the library to 3.6 and 3.7, which would eliminate the error.

@drvinceknight ok with you to drop 3.5 and add 3.7?

Edit: See #1235

[gaffney2010]

I played around with adding a test of the coded memory depth against the output of the algorithm in TestFSMPlayer so I opened gaffney2010#1. As you said there is basically no time cost to doing that so perhaps nice to include? I went through your conversations with @marcharper and didn't see that being discussed so perhaps I've missed something (feel free to close the PR or implement your own way).

I was thinking to do this in a separate PR, but I hadn't realized that it would be such a minor change. I accepted your PR into this change; it looks good to me.

One other request I'd have would be to (briefly) document the usage of the algorithm in https://axelrod.readthedocs.io/en/stable/tutorials/advanced/index.html (just a super short use case example).

I think it makes sense to make a FSM section on that page, and mention it on there. It would feel out-of-place otherwise. I'd be happy to write such a section; let me know what you think.

Finally, is it perhaps worth moving the code to it's own module (python file) to keep finite_state_macines.py for players? I personally feel it would clear things up slightly as there is currently a lot of code at the top of finite_state_machines.py that's not relevant for someone "just" contributing a strategy. Perhaps axelrod/compute_finite_state_machine_memory.py (the specific tests would be moved to unit)? What do you think?

Makes sense to me. I moved it.

[drvinceknight]

I think it makes sense to make a FSM section on that page, and mention it on there. It would feel out-of-place otherwise. I'd be happy to write such a section; let me know what you think.

Sounds good to me 👍

[drvinceknight]

For a nice overview of FSMs (with the memory length use case) perhaps this section of the docs would be more appropriate: https://axelrod.readthedocs.io/en/stable/tutorials/further_topics/index.html

?

[gaffney2010]

I decided to leave it on the further topics page, and focus on the way you execute the code. I think the research topics page would be better if I focused on open questions or some of the research around this, but this is sorta how dojo documentation approaches it already.

I put this under a broader "meta-strategies" heading, because I thought we could eventually add write-up for things like HMMPlayer, SequencePlayer, LookerUp, and Gambler. I wrote the section that it would be general enough to apply to games other than IPD, in anticipation of 5.0.

[drvinceknight]

This all looks really good to me @gaffney2010, I've left some minor comments to ensure the docs build, a style suggestion/request for the orderered memit and also a request to modularise some of the tests (sorry for being a pain).

Once these minor tweaks have been made I think this is good to go makes for an awesome contribution. 👍

[marcharper]

Here's the build error (it's in the docs):

Warning, treated as error:
/home/travis/build/Axelrod-Python/Axelrod/docs/tutorials/further_topics/meta_strategies.rst:20:citation not found: Harper2017
make: *** [html] Error 2
The command "cd docs; make clean; make html" exited with 2.

[drvinceknight]

It's the bib item missing: #1233 (comment) 👍

[gaffney2010]

Can't figure out why I can't commit that file. Ended up editing it on Github's GUI.

[gaffney2010]

I was accidentally making changes to the wrong branch. Got it cleaned up now.

[drvinceknight]

Awesome, thank you for all the changes @gaffney2010 and thanks for the overall contribution: very very cool.

I'll let @marcharper merge just to be sure he's not unhappy with any of the changes made. 👍

[drvinceknight]

I'll aim to put a new release together tomorrow 👍

[langner]

Hey guys, I just had the pleasure of reading through this thread and wanted to send a shout out. It's a very cool change. And it's encouraging to see the project being so vibrant with so much collaboration :) kudos!