pickling for strategies and transformed strategies #1092

eric-s-s · 2017-07-27T12:17:01Z

had to change test_fingerprint.py because the axl.seed behavior has changed for DualTransformer. DualTransformer still passes all its own tests.

The equality testing to Game was added so that I could test pickle.loads(pickle.dumps(player)) == player. Since it creates a new Game instance, these tests evaluated to false until i added the __eq__ method to Game.

drvinceknight · 2017-07-27T17:17:00Z

Thanks for this @eric-s-s, I've only scanned for now but so that I understand the context: this is a first step towards fixing #718 by making everything (including decorated strategies) pickable? Pickalable? Made possible to become pickles 😆 ?

drvinceknight · 2017-07-27T17:18:10Z

axelrod/tests/unit/test_pickling.py

+
+
+@st.JossAnnTransformer((0.2, 0.2), name_prefix=None)
+class JossAnn(axl.Cooperator):


What are these for?

There seem to be roughly 3 cases.

a decorated class declaration with no name prefix

based off of Player

based off of another strategy

a decorated class declaration with a name prefix

see DoubleFlip for an example

a dynamic wrapping of a class (with a name prefix)

I tried this: FlipTransformer(name_prefix=None)(Cooperator)() . It didn't work, and i was going to try to solve it, when i thought, "oh come on! No one would possible try that!"

i wanted to make sure that each transformer was tested on case-3 and either case-1 or case-2. So these are module level class declarations for case-1, dynamically wrapped strategies for case-3, and some other module level class declarations for special cases and case-2.

and i have a blind-spot for testing overkill (this is already edited down from what i used for testing the changes). problems popped up in strange ways and i got paranoid. pickling problems are weird and have little useful stack trace information when they throw an error. also, i tend to naturally think, "i wonder if you could do this stupid and unintended thing with the library? i think i'll test for that."

Ok great thanks 👍

eric-s-s · 2017-07-27T17:34:36Z

@drvinceknight yes. that's correct. Since windows multiprocess requires everything be pickled, that was the aim.

pickable? Pickalable? Made possible to become pickles 😆 ?

yeah, i've settled for "pickle-able". it does roll off the tongue, though. i've had Peter Piper running through my head for a few days now.

drvinceknight · 2017-07-27T17:36:18Z

Since windows multiprocess requires everything be pickled, that was the aim.

OK great thanks, that makes sense (and useful to have anyway I guess). Will try to review this first thing tomorrow 👍

drvinceknight

A couple of comments, questions and requests. Thanks for this. It's looking good as far as I can tell.

drvinceknight · 2017-07-28T06:43:35Z

axelrod/tests/unit/test_strategy_transformers.py

+
+        self.assertTrue(is_strategy_static(NewCooperator))
+        self.assertFalse(is_strategy_static(NewAlternator))
+
    def test_all_strategies(self):
        # Attempt to transform each strategy to ensure that implementation


I guess you were going to add this in?

you mean that i forgot to test IdentityTransformer, right? oops.

oh no!!!! it fails! i think i know why, and it may or may not be possible to fix. yikes!

arrrrggghh. brought low by the lowliest of transformers!!!

Woops my comment was actually a mistake because I misread the diff but if you've found a fail case that's good I guess although fingers crossed we can get a fix 😕 :)

phew. fixed. just needed to sleep on it. And it helped me make the code cleaner. :)

drvinceknight · 2017-07-28T06:48:50Z

axelrod/tests/unit/test_pickling.py

+    def test_pickling_all_strategies(self):
+        for s in axl.strategies:
+            player = s()
+            player.play(axl.Cooperator())


Could we wrap this in a little loop (and use Alternator):

opponent = axl.Alternator() for _ in range(5): player.play(opponent)

what about just using assert_mutated_instance_the_same_as_pickled from line 168?

drvinceknight · 2017-07-28T06:52:45Z

axelrod/strategy_transformers.py

    if not player.history:
-        player.original_player = player.original_class(**player.init_kwargs)
+        player.use_history = []


Could we call this player.original_player_history?

Actually @eric-s-s , I'm not sure this is going to work.

The reason we originally didn't just take the history but the player itself is that the Dual must keep track of the state of the original player, so that must include any other internal variables that would get changed by the original player.

I see two course of action:

Keeping the original_player and finding a way to get it to pickle

Add tests with more complex players and check that they both pass with or without this change (no matter what this should probably happen).

Not only copy the player but copy all the attributes back and forth (least pleasing of all the options in my opinion)

@theref could you take a look at this and confirm I've got things right? It's not as simple as just using the history, we need to keep track of the whole state, I don't believe the change written here ensures that.

No matter what we should probably write a test that would catch this.

Correct, the internal state of the original player is what's important, which means all internal variables must also be retained. Another option is that it may be possible to 'rebuild' the original player from the history.

I agree that some tests definitely need to be included that ensure this.

I agree that some tests definitely need to be included that ensure this.

Perhaps a test against a player that makes use of a lot of internal variables.

i'll comment the code where state-change is recorded, and the possible hidden issue (that was also true with the original)

here's the test i just tried. assert_dual_wrapper_correct is lifted from code in test_strategy_transformers.py. (it was a separate method as I used it in a few other tests (which can be mostly be removed)).

def test_all_strategies_with_dual(self): for s in axl.strategies: self.assert_dual_wrapper_correct(s) def assert_dual_wrapper_correct(self, player_class): p1 = player_class() p2 = st.DualTransformer()(player_class)() p3 = axl.CyclerCCD() # Cycles 'CCD' axl.seed(0) for _ in range(10): p1.play(p3) p3.reset() axl.seed(0) for _ in range(10): p2.play(p3) self.assertEqual(p1.history, [x.flip() for x in p2.history])

it failed on fails = [axl.EvolvedANN , axl.EvolvedANN5, axl.EvolvedANNNoise05, axl.Golden, axl.e]

after i added a function to switch player.cooperations and player.defections (and then switch them back) they all passed.

Does this work, testing-wise?

Does this work, testing-wise?

Yes I think it does. @theref can you confirm?

I expect we would get more failures for more than 10 turns (especially for the complex FSMs) but in the interest of run time I think this is a sufficient test.

after i added a function to switch player.cooperations and player.defections (and then switch them back) they all passed.

The trick will be to get an automatic function that doesn't just switch player.cooperation and .defections but every and any attribute that can affect a strategy (take a look at the player equality method, we essentially go in and grab relevant attributes there).

tested with turns=100 . all tests pass. i'll keep it at turns=100 for the next PR. it adds about 5-10 seconds to the test.

drvinceknight · 2017-07-28T06:58:51Z

axelrod/strategy_transformers.py

+def is_strategy_static(player_class):
+    """
+
+    :param player_class: Any class that inherits from axelrod.Player


Can you write a 1 sentence description and you've used sphinx (I believe?) convention here but can you tweak that to be in line with #347.

drvinceknight · 2017-07-28T06:59:21Z

axelrod/strategy_transformers.py

+
+
+class DecoratorReBuilder(object):
+    def __init__(self):


Is this necessary? I think you can just omit the def __init__

drvinceknight · 2017-07-28T06:59:44Z

axelrod/strategy_transformers.py

+
+
+class StrategyReBuilder(object):
+    def __init__(self):


Same comment? (I might be wrong and this might be necessary)

drvinceknight · 2017-07-28T07:00:10Z

axelrod/strategy_transformers.py

+        return decorator_class(*args, **kwargs)
+
+
+class StrategyReBuilder(object):


Could you write a docstring for this.

drvinceknight · 2017-07-28T07:00:16Z

axelrod/strategy_transformers.py

+            return isinstance(method, staticmethod)
+
+
+class DecoratorReBuilder(object):


Doc string please.

drvinceknight · 2017-07-28T07:03:32Z

axelrod/strategy_transformers.py

-                if is_static:
-                    # static method
-                    proposed_action = PlayerClass.strategy(opponent)
+                if strategy_wrapper == dual_wrapper:


You've lost me here, isn't this just setting the proposed_action of anything wrapped with Dual to be C?

it is doing what you said.

but more importantly, it's avoiding calling the wrapped player twice. Dual wrapper requires a proposed_action variable, but doesn't use it, the C is just a stand-in value.

The new Dual changes the state of the actual player when it calls the strategy. Calling the strategy again for proposed_action would change the state twice for one play.

Sorry but I still don't see why this is necessary and/or follow. If it's just a minor efficiency boost then lets remove it in the interest of clarity.

(see the explanation of how the state change works first)

wp = wrapped_player
op = original_player

the calls inside dual_wrapper call op.strategy on wp. this mutates the state of wp as if it was op . if the op.strategy gets called here again, it will again mutate the state of wp. this would be bad.

ex of bad way (if uses proposed_action = PlayerClass.strategy(opponent):

player = DualCycler('CD') not yet played
expected_action = C.flip() = D
cycle now advanced to D
expected_player_state = next_action_to_be_returned: D.flip()

wp.strategy called

proposed_action calls op.strategy

returns C (which will be discarded)

mutates state of wp (cycle now advanced to 'D')

dual_wrapper called

dual_wrapper calls op.strategy

returns D

mutates state of wp (cycle now advanced to C, the wrong state)

return action.flip() (which is now C, the wrong action)

removing steps 2, 3, 4 makes wp return the correct action and stay in the correct state.

drvinceknight · 2017-07-28T07:06:00Z

axelrod/tests/unit/test_strategy_transformers.py

@@ -15,6 +15,20 @@ class TestClass(object):

 class TestTransformers(unittest.TestCase):


Could we also add specific unit tests for those two extra classes you wrote in strategy_transformers: DecoratorReBuilder and StrategyReBuilder?

This has been done: 👍

drvinceknight

I believe there's a problem with your Dual transformer change.

drvinceknight · 2017-07-28T10:51:56Z

axelrod/strategy_transformers.py

    if not player.history:
-        player.original_player = player.original_class(**player.init_kwargs)
+        player.use_history = []


Actually @eric-s-s , I'm not sure this is going to work.

The reason we originally didn't just take the history but the player itself is that the Dual must keep track of the state of the original player, so that must include any other internal variables that would get changed by the original player.

I see two course of action:

Keeping the original_player and finding a way to get it to pickle

Add tests with more complex players and check that they both pass with or without this change (no matter what this should probably happen).

Not only copy the player but copy all the attributes back and forth (least pleasing of all the options in my opinion)

eric-s-s · 2017-07-28T16:39:25Z

@drvinceknight i'll get to work on all of that in the next few days. just imagine a thumbs-up on every comment

eric-s-s · 2017-07-28T17:00:55Z

axelrod/strategy_transformers.py

+
+    temp = player.history[:]
+    player.history = player.use_history[:]
+


here's where the state-change is happening. when the player calls its original_class.strategy it still affects all the state in the player. so, for instance, a DualFSMPlayer would still own an FSM, and update its state based on the call to self.FSMPlayer.strategy.

Yes but that's the problem, it should have the state of the original player.

Here's my thinking process:

op = original_player
wp = wrapped_player

play only mutates history, cooperations, defections, state_distributions.

all other player attributes are mutated in player.strategy

play_attrs = history, cooperations, defections, state_distributions

wp strategy:

switch to op.play_attrs

play op strategy

this will update wp state as if it was op

update history with op action - this step could be skipped and done on-the-fly in step 1 (see code below)

switch back to wp.play_attrs

flip action

return (assuming play was called, it will now update wp.play_attrs. this assumption was also implicit in the original implementation.)

the resulting player is a wp with wp history but op state.

code to switch play_attrs (as yet untested. This is just to get the gist):

def switch_play_attrs(player): new_history = [action.flip() for action in player.history] player.history = new_history temp = player.cooperations player.cooperations = player.defections player.defections = temp new_distributions = defaultdict(int) for key, val in player.state_distribution.items(): new_key = (key[0].flip(), key[1]) new_distributions[new_key] = val player.state_distribution = new_distributions

I think this looks right.

To ensure I'm following: step 5 and 6 could be swapped? (Not suggesting they are, just checking I'm following).

and step 3 is the important one right? Step 3 is the step that's ensuring the state is updated "properly".

To ensure I'm following: step 5 and 6 could be swapped? (Not suggesting they are, just checking I'm following).

yes, that is correct.

and step 3 is the important one right? Step 3 is the step that's ensuring the state is updated "properly"

yes. step 3 automatically happens as a result of step 2. if it didn't, this would all fall apart.

i'm in the middle of refactoring, but perhaps the new dual_wrapper code (doc_str edited out) will make things more clear.

def flip_wrapper(player, opponent, proposed_action): flip_play_attributes(player) if is_strategy_static(player.original_class): action = player.original_class.strategy(opponent) else: action = player.original_class.strategy(player, opponent) flip_play_attributes(player) return action.flip()

flip_play_attributes flips history, cooperations, defections and (sort of) state_distribution

eric-s-s · 2017-07-30T10:23:35Z

i've hit a major snag with dual and could use some advice.

first, the old version of dual has a bug. The following test passes.

    # (on master)

    def test_bug(self):
        p = IdentityTransformer()(DualTransformer()(axelrod.Cooperator))()
        p2 = DualTransformer()(axelrod.Cooperator)()

        for _ in range(3):
            p.play(p2)

        self.assertEqual(p.history, [C, C, C])
        self.assertEqual(p2.history, [D, D, D])

DualDualCooperator returns C's
DualDualDualCooperator returns D's

but if any of the Dual's are not in the outer group of decorators, it doesn't work.

DualIdentityDualCooperator returns D's
IdentityDualDualCooperator returns D's

I have a feeling that if i can understand what's going on there, it'll help solve the current problem.

current problem:
dual_wrapper calls player.original_class, which will always be the top class in a group of decorators.

so if you have FlippedDualCooperator , dual_wrapper tries to play the strategy for DualCooperator, which tries to play the strategy for DaulCooperator ... and then you reach maximum recursion depth.

current "solution" (shudder):
give the class a dedicated attr for dual_wrapper (currently called "for_dual"), that allows the dual_wrapper to call the correct strategy. Now, FlippedDualCooperator flips the DualCooperator which uses the "for_dual" attribute to do the opposite of a Cooperator. It's GREAT! PROBLEM SOLVED! except if Dual is used twice.

i've tried:

making dual_wrapper a class that owns an "original_player". (kinda-ugly, didn't work but probably the best direction)
making the new player class own a list of classes to call for dual_wrappers (very ugly, couldn't get it to work)
in the player_class, dynamically creating numbered attributes ("for_dual_1", "for_dual_2") for each dual_wrapper (super ugly and couldn't get it to work)

I would love some ideas. and/or i would love to know what's going on in the original.

drvinceknight · 2017-07-30T10:27:00Z

Am I correct in understanding that this bug "only" affects using the Dual twice?

eric-s-s · 2017-07-30T10:40:53Z

yes, that is correct.

drvinceknight · 2017-07-30T10:45:37Z

yes, that is correct.

I would be quite tempted to simply include a catch if that's ever called and raise an error. From the use case point of view being able to call them twice is not really necessary (these Duals are mainly used in fingerprinting). Obviously ideally and for completeness it would be nice to have but if this makes the code too ugly/hard to maintain and/or is not possible I'd be happy to just raise an error saying "Calling the Dual recursively is not implemented."

eric-s-s · 2017-07-30T10:49:52Z

hmmmmm. i've found a solution that works beautifully, but is very programmatically questionable (it passes a set of ugly test)

            def strategy(self, opponent):
                if strategy_wrapper == dual_wrapper:
                    
                    flip_play_attributes(self)

                if is_strategy_static(PlayerClass):
                    proposed_action = PlayerClass.strategy(opponent)
                else:
                    proposed_action = PlayerClass.strategy(self, opponent)

                if strategy_wrapper == dual_wrapper:
                    
                    flip_play_attributes(self)

                # Apply the wrapper
                return strategy_wrapper(self, opponent, proposed_action,
                                        *args, **kwargs)

mixed with

def dual_wrapper(player, opponent: Player, proposed_action: Action) -> Action:
    """Wraps the players strategy function to produce the Dual.

    The Dual of a strategy will return the exact opposite set of moves to the
    original strategy when both are faced with the same history.

    A formal definition can be found in [Ashlock2010]_.
    http://doi.org/10.1109/ITW.2010.5593352

    Parameters
    ----------
    player: Player object or subclass (self)
    opponent: Player object or subclass
    proposed_action: axelrod.Action, C or D
        The proposed action by the wrapped strategy

    Returns
    -------
    action: an axelrod.Action, C or D
    """

    # flip_play_attributes(player)
    #
    # if is_strategy_static(player.for_dual):
    #     action = player.for_dual.strategy(opponent)
    # else:
    #     action = player.for_dual.strategy(player, opponent)
    #
    # flip_play_attributes(player)

    return proposed_action.flip()

so, the player_class is responsible for all the work of the dual_wrapper. in this case, it passes all the tests that original dual_wrapper failed on and the tests that the new dual_wrapper failed on.

eric-s-s · 2017-07-30T10:52:32Z

it adds a specialized code for dual_wrapper in the strategy (which was there anyways, just less intrusively) and removes the need for a special attribute added to the dynamically created player_class. so it solves the problem better, makes one part neater and another part uglier.

drvinceknight · 2017-07-30T10:54:30Z

so, the player_class is responsible for all the work of the dual_wrapper. in this case, it passes all the tests that original dual_wrapper failed on and the tests that the new dual_wrapper failed on.

Ok I my initial thought it that this is a nice/ok solution. We should document in the dual wrapper why it has been implemented differently.

eric-s-s · 2017-07-30T10:58:05Z

Ok I my initial thought it that this is a nice/ok solution. We should document in the dual wrapper why it has been implemented differently.

i'll push that up soon and we can see how it looks

drvinceknight · 2017-07-30T12:11:07Z

i'll push that up soon and we can see how it looks

I'll review properly tomorrow 👍 Thanks :)

eric-s-s · 2017-07-30T12:35:51Z

axelrod/strategy_transformers.py

                    proposed_action = PlayerClass.strategy(opponent)
                else:
                    proposed_action = PlayerClass.strategy(self, opponent)

+                if strategy_wrapper == dual_wrapper:
+                    # after dual_wrapper_figures calls the strategy, it returns


typo here - "dual_wrapper_figures" should be "dual_wrapper". i'll wait on a full review before pushing this change.

eric-s-s · 2017-07-30T12:42:15Z

axelrod/tests/unit/test_pickling.py

+        klass = st.FlipTransformer()(axl.Cooperator)
+        klass = st.DualTransformer()(klass)
+        player = st.FinalTransformer((C, D))(klass)()
+        copy = pickle.loads(pickle.dumps(player))


this should be self.assert_original_equals_pickled(player)

drvinceknight

This is looking good.

If you could address the minor things I've left and I'd appreciate it if @theref (original author) could take a look at the way you've done the Dual just to make sure I'm not missing anything but you've written a great set of tests so I'm pretty sure we're fine.

I'm correct that test_dual_transformer_special_cases_1 and test_dual_transformer_special_cases_2 are the test cases that need to the Dual re write yes?

drvinceknight · 2017-07-31T06:43:05Z

axelrod/strategy_transformers.py

+                    return self_.__class__, (), self_.__dict__
+
+                decorators = []
+                for klass in self_.__class__.mro():


class_ perhaps?

or child_class or sub_class might be better?

Fine with that also.

drvinceknight · 2017-07-31T06:43:16Z

axelrod/strategy_transformers.py

@@ -141,6 +153,27 @@ def __repr__(self):
                    prefix = ', '
                return name

+            def reduce_for_decorated_class(self_):


Docstring please.

while checking PEP8 about klass , i noticed about the importance of sticking with self and cls. even though it shadows a variable from outer scope, is it better to shadow the self variable than to use self_ ?

here's the quote

Function and method arguments
Always use self for the first argument to instance methods.
Always use cls for the first argument to class methods.

I'm not terribly sure to be honest. I'm happy as is, I'm guessing that quote does not necessarily apply to nested def and class?

drvinceknight · 2017-07-31T06:44:28Z

axelrod/strategy_transformers.py

+                class_module = import_module(self_.__module__)
+                import_name = self_.__class__.__name__
+
+                if player_can_be_pickled(self_):


Instead of this can we use a try/except?

i'm not sure how to do it here. no error is raised in the __reduce__ function, so it wouldn't be able to catch it, right? since this is called by the pickler, that is where you'd need the try/catch.

Ok sounds like you've already thought about it. 👍

drvinceknight · 2017-07-31T06:52:39Z

axelrod/tests/unit/test_pickling.py

+            self.assert_orignal_equals_pickled(player)
+
+    def test_created_on_the_spot_multiple_transformers(self):
+        klass = st.FlipTransformer()(axl.Cooperator)


class_? (not terribly insistent on this)

i picked "klass" as it seems to be convention in a lot of code. i also have no real preference.

then i searched on PEP8 and here's what i found.

If a function argument's name clashes with a reserved keyword, it is generally better to append a single trailing underscore rather than use an abbreviation or spelling corruption. Thus class_ is better than clss . (Perhaps better is to avoid such clashes by using a synonym.)

so class_ (or something more verbose) is the official answer. Learn something new everyday!

drvinceknight · 2017-07-31T06:54:24Z

axelrod/tests/unit/test_pickling.py

+
+    def assert_original_plays_same_as_pickled(self, player, turns=10):
+        copy = pickle.loads(pickle.dumps(player))
+        opponent_1 = axl.CyclerCCCDCD()


Can we loop this over a collection of opponents:

for opponent_class in [axl.Defector(), axl.Cooperator(), axl.Random(), axl.CyclerCCCDC]:

drvinceknight · 2017-07-31T06:54:44Z

axelrod/tests/unit/test_pickling.py

+    def assert_mutated_instance_same_as_pickled(self, player):
+        player.reset()
+        turns = 5
+        opponent = axl.Alternator()


Same comment about looping over a set of opponents.

on second look, these two tests can be combined and save some time (which will be taken up and more with extra strategies). i'll just run

def assert_equals_instance_from_pickling(original_instance): copy = pickle.loads(pickle.dumps(original_instance)) self.assertEqual(copy, original_instance)

after every Match. i'll have a nice batch of already mutated instances.

Sounds good.

drvinceknight · 2017-07-31T06:57:47Z

axelrod/tests/unit/test_pickling.py

+        self.assert_mutated_instance_same_as_pickled(player)
+
+    def test_dual_transformer_special_case(self):
+        player = TestDualTransformerIssues()


This isn't terribly clear? (even the class definition before)

Could the class def be moved here and made a bit more explicit.

unfortunately, this can't be moved inside the test. no pickling of non-module-lvl classes. i'll work on clarity in class name, test name, and perhaps comments.

drvinceknight · 2017-07-31T06:59:31Z

axelrod/tests/unit/test_strategy_transformers.py


-        self.assertEqual(p1.history, [x.flip() for x in p2.history])
+    def test_dual_transformer_special_cases_1(self):


Am I right that these tests are the ones that are fixed by your re write of Dual?

yes, that is correct. it addresses issues with the original code and a few different tries on the re-write. i'll try to make this more clear.

drvinceknight

I've left one small stylistic comment for a crazy set of nested function calls but that's not a deal breaker for me.

I think this is basically good to go now. I would still like @theref to take a look and approve the change to DualTransformer.

drvinceknight · 2017-08-01T06:07:38Z

axelrod/tests/unit/test_pickling.py

        self.assert_orignal_equals_pickled(player)

+        interspersed_dual_transformers = (


This isn't terribly nice to look at. Perhaps we can make use of intermediate variables to improve readability?

i wasn't sure how to go about it. i'll do that.

drvinceknight · 2017-08-01T06:11:05Z

axelrod/tests/unit/test_strategy_transformers.py

@@ -15,6 +15,20 @@ class TestClass(object):

 class TestTransformers(unittest.TestCase):


This has been done: 👍

theref · 2017-08-01T16:16:20Z

axelrod/tests/unit/test_strategy_transformers.py

+        multiple_dual_transformers = DualTransformer()(DualTransformer()(axelrod.WinStayLoseShift))
+        self.assert_dual_wrapper_correct(multiple_dual_transformers)
+
+    def assert_dual_wrapper_correct(self, player_class):


Yeah I really like this test, great work!

Thanks @theref 👍

i'd love to take credit, but my contribution to this particular test was pulling it out of the original testing code to make a function and adding axl.seed(0). whoever wrote the original (test_dual_somestrategy) made a nice concise test that showed me exactly what dual_transformer needed to do. if that was you, @theref , thanks!

marcharper

Looks good to me -- just a couple of typos / docstring requests. Thanks for taking this on!

marcharper · 2017-08-02T00:17:10Z

axelrod/strategy_transformers.py

@@ -9,9 +9,12 @@
 import copy
 import inspect
 import random
+from typing import Any


Please reorder the imports (alphabetical, std lib, then 3rd party, then project specific)

good to know!

marcharper · 2017-08-02T00:19:09Z

axelrod/strategy_transformers.py

@@ -57,6 +60,14 @@ def __init__(self, *args, **kwargs):
            else:
                self.name_prefix = name_prefix

+        def __reduce__(self):


Can we add a docstring here? I get that we're rebuilding the class but if I didn't know what was going on I wouldn't know until I read through the file, and this is a fairly complicated part of the library so I'd like to err on the side of more documentation.

marcharper · 2017-08-02T00:21:14Z

axelrod/strategy_transformers.py

                    proposed_action = PlayerClass.strategy(opponent)
                else:
                    proposed_action = PlayerClass.strategy(self, opponent)

+                if strategy_wrapper == dual_wrapper:
+                    # after dual_wrapper calls the strategy, it returns


Capital After

marcharper · 2017-08-02T00:24:24Z

axelrod/strategy_transformers.py

+    Flips all the attributes created by `player.play`:
+        - `player.history`,
+        - `player.cooperations`,
+        - `player.defectsions`,


marcharper · 2017-08-02T00:29:30Z

@eric-s-s Please rebase onto master.

eric-s-s · 2017-08-02T14:51:28Z

@marcharper

did the rebase work correctly? it's the first time i tried it and i suspect it did not.

commit on my branch
checkout master, pull upstream
checkout my branch
git rebase master
follow all merge-conflict instructions (following what's in master)
when this was done, i was somehow behind my branch. followed resolve instructions

as far as i can tell, all my changes are here. i have no idea what is going on with all the other changed files, though. should i not have pulled from upstream before doing this?

marcharper · 2017-08-03T01:37:13Z

I'm not sure what you actually did -- it looks like there are twice as many commits. When I tried to rebase again I got a ton of merge conflicts, so I suspect that you somehow rewrote the commits then pushed them to the head of the old branch, while picking up some old commits. Maybe you rebased master onto your branch?

I was able to squash all the commits, see #1106. I think that #1107 has the original commits, rebased.

eric-s-s · 2017-08-03T04:43:51Z

for future reference, did steps 1-4 follow correct procedure? on the git-rebase man-page it looked so simple and straightforward :( .

i reset the head and tried it again. i had the same merge conflicts and resolved in the same way. looking at the git log, it looks fine. perhaps it was step 6? when i went to push up the rebased branch, i had a conflict and git suggested that i git pull. should i have pushed up the rebased branch as a new PR?

drvinceknight · 2017-08-03T07:22:16Z

should i have pushed up the rebased branch as a new PR?

Yeah I think that was the error, you should have git push -f git force pushed and just over written this branch or indeed just opened a second PR :)

eric-s-s · 2017-08-03T11:11:06Z

@drvinceknight thanks.

i reset the head, re-rebased it and am going to try that now.

looking at the insghts/graphs/network and the files_changed, it seems to have worked. if that's the case, maybe merge this one and close the other two?

marcharper · 2017-08-03T13:54:49Z

I think this is fine to merge now and we can close the others.

drvinceknight · 2017-08-03T17:40:47Z

I'm just going to re start the builds to triple check nothing got lost during the rebases.

drvinceknight · 2017-08-03T18:57:11Z

Thanks @eric-s-s 👍 Great work getting to the bottom of this. 👍

eric-s-s · 2017-08-03T19:41:43Z

it was my pleasure. :)

drvinceknight reviewed Jul 27, 2017

View reviewed changes

drvinceknight requested changes Jul 28, 2017

View reviewed changes

drvinceknight requested a review from theref July 28, 2017 13:21

eric-s-s commented Jul 28, 2017

View reviewed changes

eric-s-s commented Jul 30, 2017

View reviewed changes

drvinceknight requested changes Jul 31, 2017

View reviewed changes

drvinceknight approved these changes Aug 1, 2017

View reviewed changes

theref approved these changes Aug 1, 2017

View reviewed changes

drvinceknight added the ready-to-merge label Aug 1, 2017

marcharper approved these changes Aug 2, 2017

View reviewed changes

basic strategies ok

e6619bc

eric-s-s added 19 commits August 3, 2017 13:19

tests commited

c31d578

check for names

2035a62

branch switch

560ca9c

tests pass. still a mess

89b8588

made dual pickle-able

8048f2f

branch switch

3591bd9

just to upload picture

a15ee47

before change to decorator __reduce__

9138624

ready to push

8a39986

added game test. fixed docstring issues

6bee0d6

fixed fingerprint tests

7b786ed

branch switch

5ef9dc6

mostly fixed. branch switch

7ec8d8d

solves dual problem for single dual

d0382eb

a solution for dual_wrapper

4313a19

test fixes. branch switch

f9d5d7c

improved assert_original_equals_pickled. small fixes

e8cc987

style fix in one test_pickling test

f1001e0

small typos

4ef70f9

drvinceknight mentioned this pull request Aug 3, 2017

Eric-s-s orig #1107

Closed

eric-s-s force-pushed the side_reduce branch from ad8d5c7 to 4ef70f9 Compare August 3, 2017 11:11

drvinceknight merged commit 08f0c1f into Axelrod-Python:master Aug 3, 2017

eric-s-s deleted the side_reduce branch August 3, 2017 19:41



		@st.JossAnnTransformer((0.2, 0.2), name_prefix=None)
		class JossAnn(axl.Cooperator):

		return decorator_class(args, *kwargs)


		class StrategyReBuilder(object):

		return isinstance(method, staticmethod)


		class DecoratorReBuilder(object):

		@@ -15,6 +15,20 @@ class TestClass(object):

		class TestTransformers(unittest.TestCase):


		temp = player.history[:]
		player.history = player.use_history[:]


		self.assertEqual(p1.history, [x.flip() for x in p2.history])
		def test_dual_transformer_special_cases_1(self):

		self.assert_orignal_equals_pickled(player)

		interspersed_dual_transformers = (

pickling for strategies and transformed strategies #1092

pickling for strategies and transformed strategies #1092

Conversation

eric-s-s commented Jul 27, 2017 • edited

drvinceknight commented Jul 27, 2017 • edited

Choose a reason for hiding this comment

eric-s-s Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s commented Jul 27, 2017

drvinceknight commented Jul 27, 2017

drvinceknight left a 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

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

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

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

eric-s-s Jul 28, 2017 • edited

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

drvinceknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s Jul 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight Jul 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s Jul 29, 2017 • edited

Choose a reason for hiding this comment

eric-s-s commented Jul 30, 2017

drvinceknight commented Jul 30, 2017

eric-s-s commented Jul 30, 2017

drvinceknight commented Jul 30, 2017

eric-s-s commented Jul 30, 2017

eric-s-s commented Jul 30, 2017

drvinceknight commented Jul 30, 2017

eric-s-s commented Jul 30, 2017

drvinceknight commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s Jul 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-s-s Jul 31, 2017 • edited

eric-s-s commented Jul 27, 2017 •

edited

drvinceknight commented Jul 27, 2017 •

edited

eric-s-s Jul 27, 2017 •

edited

eric-s-s Jul 28, 2017 •

edited

eric-s-s Jul 29, 2017 •

edited

drvinceknight Jul 29, 2017 •

edited

eric-s-s Jul 29, 2017 •

edited

eric-s-s Jul 31, 2017 •

edited

eric-s-s Jul 31, 2017 •

edited

eric-s-s Jul 31, 2017 •

edited

eric-s-s Jul 31, 2017 •

edited