Bet changes Supersedes #19

[amortization]

Couldn't find a way to updated #19 so created this one instead.

[amortization]

@amortization From the looks of it, both the ironcross() and hammerlock() strategies are slightly off now. I fixed the tests in 50101e6, but didn't fix the strategies yet. For the current code to work, I think there needs to be a unit argument to the core strategies like place.

Also, can you update amortization:bet_changes to skent259:bet_changes, so the files changed load correctly on this? Thank you! I'll do a full review after that, but I think this is getting close

Could we remove unit from player altogether and have each strategy define it's own parameters for running? Take a look at the implementation of this in #29. That way for different bets we could have different parameters for unit not reliant on a single integer, ex. for PassLineOdds we have {4: 5, 5: 5, 6: 6, 8: 6, 9: 5, 10: 5} which indicates the amount of bet for each odds number. If the unit was 30 for instance, a player probably wouldn't need to bet 36, they could just bet the 30 since the payout comes out to an even number anyways.

[skent259]

Could we remove unit from player altogether and have each strategy define it's own parameters for running? Take a look at the implementation of this in #29. That way for different bets we could have different parameters for unit not reliant on a single integer, ex. for PassLineOdds we have {4: 5, 5: 5, 6: 6, 8: 6, 9: 5, 10: 5} which indicates the amount of bet for each odds number. If the unit was 30 for instance, a player probably wouldn't need to bet 36, they could just bet the 30 since the payout comes out to an even number anyways.

Yes, that makes a lot of sense. The more I think about it, players don't really have a unit that they bet, it's the strategies that define the bet amounts. Better to have the argument in there. I like bet_amount or amount rather than unit.

I like that some strategies could have a different argument than this, but I'm thinking that until we implement #29, it might be easiest to just have the same argument for most (if not all) strategies.

[amortization]

Could we remove unit from player altogether and have each strategy define it's own parameters for running? Take a look at the implementation of this in #29. That way for different bets we could have different parameters for unit not reliant on a single integer, ex. for PassLineOdds we have {4: 5, 5: 5, 6: 6, 8: 6, 9: 5, 10: 5} which indicates the amount of bet for each odds number. If the unit was 30 for instance, a player probably wouldn't need to bet 36, they could just bet the 30 since the payout comes out to an even number anyways.

Yes, that makes a lot of sense. The more I think about it, players don't really have a unit that they bet, it's the strategies that define the bet amounts. Better to have the argument in there. I like bet_amount or amount rather than unit.

I like that some strategies could have a different argument than this, but I'm thinking that until we implement #29, it might be easiest to just have the same argument for most (if not all) strategies.

So looking at the usages of Player.unit a little bit, it looks like if we do this we will re-introduce issue #5 since there will be no unit on the player to look at for a stopping condition...

I think the long term solution is to have a stopping condition in Strategy in #29 that has a default for most strategies, but can be overridden by a user. ex:

class BetIfTrue(Strategy):
...
    def stopping_condition(self, player: "Player") -> bool:
        return self.bet.bet_amount > player.bankroll

That way the user can set up when they want it to stop themselves. For example, if they want to stop when they win x amount of money, when they get x amount of rewards points, when they have wagered x amount, etc. I think that this would provide useful flexibility for users depending on their unique use cases.

For now it maybe makes sense to put the unit in strat_info? But it will require end users to do player.strat_info['amount'] = x in all of their strategies otherwise it will re-introduce #5.

[skent259]

So looking at the usages of Player.unit a little bit, it looks like if we do this we will re-introduce issue #5 since there will be no unit on the player to look at for a stopping condition...

I think the long term solution is to have a stopping condition in Strategy in #29 that has a default for most strategies, but can be overridden by a user. ex:

class BetIfTrue(Strategy):
...
    def stopping_condition(self, player: "Player") -> bool:
        return self.bet.bet_amount > player.bankroll

That way the user can set up when they want it to stop themselves. For example, if they want to stop when they win x amount of money, when they get x amount of rewards points, when they have wagered x amount, etc. I think that this would provide useful flexibility for users depending on their unique use cases.

For now it maybe makes sense to put the unit in strat_info? But it will require end users to do player.strat_info['amount'] = x in all of their strategies otherwise it will re-introduce #5.

Ahh, yeah I can see where that issue would return. I think you're right that #29 provides an elegant solution to this, so +1 for that idea.

For the meantime, let's go with your suggestion of putting into player.strat_info. Perhaps the line can be something like this?

player.strat_info['amount'] = min(player.strat_info['amount'], x)

where the default is set upon player initialization at float('inf'). This way if a user builds their strategy from the ones we have defined, they won't have to add the line because it will automatically calculate.

[skent259]

In thinking more about the following lines, should all() really be any()? The table should continue rolling if any of the players can bet, right?

https://github.com/skent259/crapssim/blob/23b8d76b0f8ebc3dcabecc8e71c3c4fef8686ac6/crapssim/table.py#L248-L257

[amortization]

In thinking more about the following lines, should all() really be any()? The table should continue rolling if any of the players can bet, right?

https://github.com/skent259/crapssim/blob/23b8d76b0f8ebc3dcabecc8e71c3c4fef8686ac6/crapssim/table.py#L248-L257

Yeah that probably would make more sense.

[amortization]

So looking at the usages of Player.unit a little bit, it looks like if we do this we will re-introduce issue #5 since there will be no unit on the player to look at for a stopping condition... I think the long term solution is to have a stopping condition in Strategy in #29 that has a default for most strategies, but can be overridden by a user. ex:

class BetIfTrue(Strategy):
...
    def stopping_condition(self, player: "Player") -> bool:
        return self.bet.bet_amount > player.bankroll

That way the user can set up when they want it to stop themselves. For example, if they want to stop when they win x amount of money, when they get x amount of rewards points, when they have wagered x amount, etc. I think that this would provide useful flexibility for users depending on their unique use cases. For now it maybe makes sense to put the unit in strat_info? But it will require end users to do player.strat_info['amount'] = x in all of their strategies otherwise it will re-introduce #5.

Ahh, yeah I can see where that issue would return. I think you're right that #29 provides an elegant solution to this, so +1 for that idea.

For the meantime, let's go with your suggestion of putting into player.strat_info. Perhaps the line can be something like this?

player.strat_info['amount'] = min(player.strat_info['amount'], x)

where the default is set upon player initialization at float('inf'). This way if a user builds their strategy from the ones we have defined, they won't have to add the line because it will automatically calculate.

I'll probably let you take a look at doing that, I'm not quite 100% sure how you were looking to do that but it seems like you have something in mind. I was thinking more along the lines of adding that as an optional parameter to each strategy somehow.

[skent259]

I'll probably let you take a look at doing that, I'm not quite 100% sure how you were looking to do that but it seems like you have something in mind. I was thinking more along the lines of adding that as an optional parameter to each strategy somehow.

I took a look and realized how complicated this is going to make all the strategies. There's really subtle ways in which a user-made strategy can go wrong and it would be hard to document all the edge cases. I don't think it's a good solution in the short or long term.

For now, let's leave player.unit alone. I'll push an update here that fixes ironcross() and hammerlock(), even if it's not the most elegant code. I'm now thinking that implementing #29 would be ideal for 0.3.0. There's a number of breaking changes anyways and we might as well get most of them out of the way.

Thoughts?

[amortization]

I'll probably let you take a look at doing that, I'm not quite 100% sure how you were looking to do that but it seems like you have something in mind. I was thinking more along the lines of adding that as an optional parameter to each strategy somehow.

I took a look and realized how complicated this is going to make all the strategies. There's really subtle ways in which a user-made strategy can go wrong and it would be hard to document all the edge cases. I don't think it's a good solution in the short or long term.

For now, let's leave player.unit alone. I'll push an update here that fixes ironcross() and hammerlock(), even if it's not the most elegant code. I'm now thinking that implementing #29 would be ideal for 0.3.0. There's a number of breaking changes anyways and we might as well get most of them out of the way.

Thoughts?

I certainly think that would make development of other things easier going forward, although I think we should probably really beef up our Strategy tests to make sure that all of the strategies that are included are working as expected. Really having full coverage for each of the methods of the strategies would be ideal. maybe even if we did some regression tests with running the strategies currently in the base branch and compared the output of those against the output of the new strategies.

I also probably want to rebase that branch from bet_changes since there are some useful changes made in bet_changes that aren't currently included in that branch so that could take some time to get right but is definitely doable.