Strategy rewrite

[amortization]

This is probably a work in process, but I I re-wrote strategy to use a Strategy object instead of numerous functions. It also includes some basic building block strategy objects that users can use to build strategies. Here are some highlights:

• The overridable after_roll method allows strategies to determine objects on a table after the roll, but prior to the bets and table being updated. This makes strategies like Place68CPR that depend on the status of bets that are placed possible without having to imply it from the state of the table.
• There is an AggregateStrategy which can be created from the add method of strategy allowing users to combine multiple strategies into one. For example, BetPassLine(5) + PassLineOdds(2) would place a 5 dollar bet on the pass line and 2x odds of the pass line bet.
• BetIfTrue takes a Bet object and a callable key which returns a boolean based on the table/player attributes to determine whether the bet should be placed. For example, BetIfTrue(PassLine(5), lambda p, t: t.point.status == 'Off') would place a pass line bet if the point is off. This works similarly to Pythons built in filter function. And would allow users to make simple Strategies based on lambdas.
• RemoveIfTrue is similar except for removing bets, ex to remove all Place bets it's RemoveIfTrue(lambda b, p, t: isinstance(b, Place)
• A lot of building blocks based on BetIfTrue: BetPointOff, BetPointOn, BetPassLine, CountStrategy, etc.
• Another building block is OddStrategy which places odds based on a multiplier and type. Ex. OddsStrategy(PassLine, 2) places 2x odds on the PassLine.
• I also included all of the existing strategies (which should be working) all based on the new building blocks, and included Place68CPR which was originally not working as well.

[amortization]

There are a couple of name and API changes I want to make since we are making breaking changes anyways but want to run them by you first before making them.

• Bet.betamount to Bet.amount I think that having the leading bet in that is a bit redundant. If amount is too generic (since I guess it could be construed as a winning amount or some other amount) I also thought Bet.wager could work as well.
• I'd like to remove the Bet.name property. I think that we'd rather have users using the bets type, or the specific bet itself for searching for bets and the name should no longer be necessary.
• For Dice I'd like to make both result and total as properties with setters doing essentially what fixed_roll does. I think that it will be a lot easier for testing to set dice.result = (4, 4) instead of having to do dice.result = (4, 4), dice.total = sum(self.result), dice.n_rolls += 1. fixed_roll essentially handles this now for the dice result, but I would also like the total to be set and it defaults in a result ex: dice.total = 7 would automatically set the dice result to some sort of 7 ex: (6, 1)
• In Table.run I think that max_shooter should be max_shooters to match up with max_rolls being plural.
• In Table.should_keep_rolling I think that runout should be run_out to be PEP8 compliant since it's two words.
• I already made this change on this branch, but I changed Point.status to a property based on Point.number so for testing you only need to set the points number, not the points status as well.
• In Point.update I think that dice_object should just be dice as it being a dice object should be implied with the docstring and the type hinting.
• Player.bet_strategy I think can just be Player.strategy as the Strategy will be implied in the type hinting.
• Player.bets_on_table I think can just be Player.bets

[skent259]

There are a couple of name and API changes I want to make since we are making breaking changes anyways but want to run them by you first before making them.

• Bet.betamount to Bet.amount I think that having the leading bet in that is a bit redundant. If amount is too generic (since I guess it could be construed as a winning amount or some other amount) I also thought Bet.wager could work as well.

Bet.amount is good. I've thought about Bet.wager before but bet and wager both sort of mean the same thing so amount is preferable

• I'd like to remove the Bet.name property. I think that we'd rather have users using the bets type, or the specific bet itself for searching for bets and the name should no longer be necessary.
• For Dice I'd like to make both result and total as properties with setters doing essentially what fixed_roll does. I think that it will be a lot easier for testing to set dice.result = (4, 4) instead of having to do dice.result = (4, 4), dice.total = sum(self.result), dice.n_rolls += 1. fixed_roll essentially handles this now for the dice result, but I would also like the total to be set and it defaults in a result ex: dice.total = 7 would automatically set the dice result to some sort of 7 ex: (6, 1)

How would the dice pick a result based on changing the total? I worry if there is ambiguity there. Because result and total are properties that are intertwined, wouldn't it make the most sense in testing to just use dice.fixed_roll()? This keeps the result explicit and I think also makes the code base and testing clearer

• In Table.run I think that max_shooter should be max_shooters to match up with max_rolls being plural.
• In Table.should_keep_rolling I think that runout should be run_out to be PEP8 compliant since it's two words.
• I already made this change on this branch, but I changed Point.status to a property based on Point.number so for testing you only need to set the points number, not the points status as well.
• In Point.update I think that dice_object should just be dice as it being a dice object should be implied with the docstring and the type hinting.
• Player.bet_strategy I think can just be Player.strategy as the Strategy will be implied in the type hinting.
• Player.bets_on_table I think can just be Player.bets

Otherwise, I agree with all of these changes. Should make the API cleaner and easier to understand.

[amortization]

• For Dice I'd like to make both result and total as properties with setters doing essentially what fixed_roll does. I think that it will be a lot easier for testing to set dice.result = (4, 4) instead of having to do dice.result = (4, 4), dice.total = sum(self.result), dice.n_rolls += 1. fixed_roll essentially handles this now for the dice result, but I would also like the total to be set and it defaults in a result ex: dice.total = 7 would automatically set the dice result to some sort of 7 ex: (6, 1)

How would the dice pick a result based on changing the total? I worry if there is ambiguity there. Because result and total are properties that are intertwined, wouldn't it make the most sense in testing to just use dice.fixed_roll()? This keeps the result explicit and I think also makes the code base and testing clearer

I see where you're coming from with setting the total, I figured it could just be:

@total.setter
    def total(self, value: int):
        self.result = (int(value / 2) + value % 2, int(value / 2))

And then you'd only have to set it for tests that depend on the specific dice rolled (eg. Hardways.)

At the very least I think the code should be this:

    def __init__(self) -> None:
        self.result: typing.Iterable[int] | None = None
        self.n_rolls: int = 0

    @property
    def total(self):
        return sum(self.result)

    def roll(self) -> None:
        self.n_rolls += 1
        self.result = r.randint(1, 7, size=2)

    def fixed_roll(self, outcome: typing.Iterable[int]) -> None:
        self.n_rolls += 1
        self.result = outcome

Since total should always be a sum of the result. Ideally if we are sticking to the Dice storing the roll history (although in some ways I think that this should be built out with the table storing more roll/bet/point etc. history) I think envisioned something like this:

    def __init__(self) -> None:
        self.rolls: list[tuple(int, int)] = []

    @property
    def result(self) -> tuple[int, int]:
        return self.rolls[-1]

    @result.setter
    def result(self, value: tuple[int, int]):
        self.rolls.append(value)

    @property
    def total(self):
        return sum(self.result)

    @total.setter
    def total(self, value: int):
        self.result = (int(value / 2) + value % 2, int(value / 2))

    @property
    def n_rolls(self):
        return len(self.rolls)

    def roll(self) -> None:
        self.result = r.randint(1, 7, size=2)

    def fixed_roll(self, outcome: typing.Iterable[int]) -> None:
        self.result = outcome

Where the rolls would then have the entire roll history for the dice. In this case you wouldn't need fixed_roll, you could just do dice.result = (4, 4) and rolls, total, result, and n_rolls would all automatically be set.

[amortization]

I'm looking for some feedback on the c92956b push. I split turned the strategy.py into a package and split it up into two modules, core and defaults. Core features strategies that can be used as building blocks for other strategies, ex. BetIfTrue, RemoveIfTrue, etc. and defaults has the strategies that are completed and can be run by the users. I think that it is important that these are separated and differentiated as the strategy.py file was over 1000 lines with the docstrings, and I wanted it to be a tighter more abbreviated and I think that this correctly separates the strategy building blocks from the completed strategies.

A couple of questions regarding those:

• How do we feel about the names core and defaults? I'm tempted to switch defaults to examples as I think that may better describe the strategies included, but was wondering the thoughts on that.
• Currently, the strategy init.py has imports for everything so the user can do from crapssim.strategy import BetIfTrue, Place68CPR. I think that we may not want that for end users as I think it might be clearer to them which ones are intended as examples vs the core building blocks. I think that it would be clearest if the user did from crapssim.strategy.core import BetIfTrue and from crapssim.strategy.examples import HammerLock. My other thoughts on this are leave the core ones in init to allow from crapssim.strategy import BetPointOn and still require the explicit path for the examples.

[skent259]

How would the dice pick a result based on changing the total? I worry if there is ambiguity there. Because result and total are properties that are intertwined, wouldn't it make the most sense in testing to just use dice.fixed_roll()? This keeps the result explicit and I think also makes the code base and testing clearer

I see where you're coming from with setting the total, I figured it could just be:

@total.setter
    def total(self, value: int):
        self.result = (int(value / 2) + value % 2, int(value / 2))

And then you'd only have to set it for tests that depend on the specific dice rolled (eg. Hardways.)

At the very least I think the code should be this:

    def __init__(self) -> None:
        self.result: typing.Iterable[int] | None = None
        self.n_rolls: int = 0

    @property
    def total(self):
        return sum(self.result)

    def roll(self) -> None:
        self.n_rolls += 1
        self.result = r.randint(1, 7, size=2)

    def fixed_roll(self, outcome: typing.Iterable[int]) -> None:
        self.n_rolls += 1
        self.result = outcome

I like this approach. To me, it's clear and to the point

Since total should always be a sum of the result. Ideally if we are sticking to the Dice storing the roll history (although in some ways I think that this should be built out with the table storing more roll/bet/point etc. history) I think envisioned something like this:

    def __init__(self) -> None:
        self.rolls: list[tuple(int, int)] = []

    @property
    def result(self) -> tuple[int, int]:
        return self.rolls[-1]

    @result.setter
    def result(self, value: tuple[int, int]):
        self.rolls.append(value)

    @property
    def total(self):
        return sum(self.result)

    @total.setter
    def total(self, value: int):
        self.result = (int(value / 2) + value % 2, int(value / 2))

    @property
    def n_rolls(self):
        return len(self.rolls)

    def roll(self) -> None:
        self.result = r.randint(1, 7, size=2)

    def fixed_roll(self, outcome: typing.Iterable[int]) -> None:
        self.result = outcome

Where the rolls would then have the entire roll history for the dice. In this case you wouldn't need fixed_roll, you could just do dice.result = (4, 4) and rolls, total, result, and n_rolls would all automatically be set.

I think storing the rolls will unnecessarily add to the memory storage. I think having independent setters for the result/total overly complicates things. In testing, the line dice.result = (4, 4) and dice.fixed_roll((4, 4)) are equally as easy.

[skent259]

I'm looking for some feedback on the c92956b push. I split turned the strategy.py into a package and split it up into two modules, core and defaults. Core features strategies that can be used as building blocks for other strategies, ex. BetIfTrue, RemoveIfTrue, etc. and defaults has the strategies that are completed and can be run by the users. I think that it is important that these are separated and differentiated as the strategy.py file was over 1000 lines with the docstrings, and I wanted it to be a tighter more abbreviated and I think that this correctly separates the strategy building blocks from the completed strategies.

A couple of questions regarding those:

• How do we feel about the names core and defaults? I'm tempted to switch defaults to examples as I think that may better describe the strategies included, but was wondering the thoughts on that.
• Currently, the strategy init.py has imports for everything so the user can do from crapssim.strategy import BetIfTrue, Place68CPR. I think that we may not want that for end users as I think it might be clearer to them which ones are intended as examples vs the core building blocks. I think that it would be clearest if the user did from crapssim.strategy.core import BetIfTrue and from crapssim.strategy.examples import HammerLock. My other thoughts on this are leave the core ones in init to allow from crapssim.strategy import BetPointOn and still require the explicit path for the examples.

I like the idea of splitting things out into separate modules. A couple thoughts on that:

• From my perspective, it might be worth having 3 files. Naming could use some work, but here's what I'm thinking:
  • "core" - logical building blocks like BetIfTrue, RemoveIfTrue, etc, like you currently have
  • "single_bet" - Things like BetPassLine, BetPlace, etc, which essentially place one bet (or one type). These are a different type of building block where a user could add to other strategies. Like if you want to do Ironcross with a fire bet, you know where to look for that strategy. Needs a better name though.
  • "examples" - more complicated iterations, trying to keep heavily to named strategies. Anything that can be repurposed I think belongs more in "core".

In terms of init, I'm less clear. I think there's an advantage to having everything available in crapssim.strategy, because it could be frustrating to try to figure out where things are. Of any, I think the examples could be most easily excluded from this

[skent259]

By the way, all of the integration testing that you've added to this is excellent. And while there's a few points in the discussion to hammer out, I think all of this effort will make building strategies much easier and clear going forward. So, thank you!

Let me know when you feel good with it and I'll do a full review to incorporate into #30 and get these changes out into 0.3.0

[amortization]

I like the idea of splitting things out into separate modules. A couple thoughts on that:

• From my perspective, it might be worth having 3 files. Naming could use some work, but here's what I'm thinking:

  • "core" - logical building blocks like BetIfTrue, RemoveIfTrue, etc, like you currently have
  • "single_bet" - Things like BetPassLine, BetPlace, etc, which essentially place one bet (or one type). These are a different type of building block where a user could add to other strategies. Like if you want to do Ironcross with a fire bet, you know where to look for that strategy. Needs a better name though.
  • "examples" - more complicated iterations, trying to keep heavily to named strategies. Anything that can be repurposed I think belongs more in "core".

In terms of init, I'm less clear. I think there's an advantage to having everything available in crapssim.strategy, because it could be frustrating to try to figure out where things are. Of any, I think the examples could be most easily excluded from this

I really like the idea of having the single bet building blocks! I think having those and core in init would make the most sense, with examples being accessible through strategy.examples.

[amortization]

I think storing the rolls will unnecessarily add to the memory storage. I think having independent setters for the result/total overly complicates things. In testing, the line dice.result = (4, 4) and dice.fixed_roll((4, 4)) are equally as easy.

The issue is that a user can result can be set without n_rolls increasing. As is, or with just having the property for total, a user can do dice.result = (4, 4) and the dice.result (and total if we have the property for total) will be correct, but (dice.n_rolls == 0) is True since the n_rolls isn't ever increased.

[skent259]

I think storing the rolls will unnecessarily add to the memory storage. I think having independent setters for the result/total overly complicates things. In testing, the line dice.result = (4, 4) and dice.fixed_roll((4, 4)) are equally as easy.

The issue is that a user can result can be set without n_rolls increasing. As is, or with just having the property for total, a user can do dice.result = (4, 4) and the dice.result (and total if we have the property for total) will be correct, but (dice.n_rolls == 0) is True since the n_rolls isn't ever increased.

I see your point. I think that a user shouldn't be able to set the dice result manually like that. To me, it makes more sense to force them to use the fixed_roll() function by making result not settable.

I'm thinking something like this:

    def __init__(self) -> None:
        self._result: typing.Iterable[int] | None = None
        self.n_rolls: int = 0

    @property
    def total(self) -> int:
        return sum(self.result)

    @property 
    def result(self) -> typing.Iterable[int]:
        return self._result

    def roll(self) -> None:
        self.n_rolls += 1
        self._result = r.randint(1, 7, size=2)

    def fixed_roll(self, outcome: typing.Iterable[int]) -> None:
        self.n_rolls += 1
        self._result = outcome

Some useful tests (if you agree, I can add)

def test_set_total(d1):
    d1.fixed_roll([3,4])
    with pytest.raises(AttributeError, match="can't set attribute 'total'") as e_info:
        d1.total = 8
    assert d1.total == 7

def test_set_result(d1):
    d1.fixed_roll([3,4])
    with pytest.raises(AttributeError, match="can't set attribute 'result'") as e_info:
        d1.result = [4,4]

[amortization]

I think storing the rolls will unnecessarily add to the memory storage. I think having independent setters for the result/total overly complicates things. In testing, the line dice.result = (4, 4) and dice.fixed_roll((4, 4)) are equally as easy.

The issue is that a user can result can be set without n_rolls increasing. As is, or with just having the property for total, a user can do dice.result = (4, 4) and the dice.result (and total if we have the property for total) will be correct, but (dice.n_rolls == 0) is True since the n_rolls isn't ever increased.

I see your point. I think that a user shouldn't be able to set the dice result manually like that. To me, it makes more sense to force them to use the fixed_roll() function by making result not settable.

I'm thinking something like this:

    def __init__(self) -> None:
        self._result: typing.Iterable[int] | None = None
        self.n_rolls: int = 0

    @property
    def total(self) -> int:
        return sum(self.result)

    @property 
    def result(self) -> typing.Iterable[int]:
        return self._result

    def roll(self) -> None:
        self.n_rolls += 1
        self._result = r.randint(1, 7, size=2)

    def fixed_roll(self, outcome: typing.Iterable[int]) -> None:
        self.n_rolls += 1
        self._result = outcome

Some useful tests (if you agree, I can add)

def test_set_total(d1):
    d1.fixed_roll([3,4])
    with pytest.raises(AttributeError, match="can't set attribute 'total'") as e_info:
        d1.total = 8
    assert d1.total == 7

def test_set_result(d1):
    d1.fixed_roll([3,4])
    with pytest.raises(AttributeError, match="can't set attribute 'result'") as e_info:
        d1.result = [4,4]

Yeah those tests look fine to me.