Changed how Odds bets work and how Bets are queried by Player

[amortization]

Branch of #19

I split out Odds into separate types and added validation logic for placing them based on whether they have a base bet type on the Table.

I also added the method get_bets to player. This method takes bet_types and retrieves all of the bets the player has of that is an instance of that type. This replaces the need for subname for Odds bets as one can either do

player.get_bets(Odds)

to get all of the Odds bets, or can do

player.get_bets(Odds6)

to get all of the Odds6 bets. it also takes a **kwargs arguments of bet attributes which would let the player query any attribute of a given bet type. For Example, if they wanted Come bets where the point is 6 they can do:

player.get_bets(Come, point=6)

whereas if they just wanted all Come bets they can do:

player.get_bets(Come)

This should work with any bet attribute and any Bet type or parent type. For example if you wanted all where the status is "win" you can just do:

player.get_bets(status=win)

I think that this will allow our users to have more robust lookups of bets on the table, since they will be able to get_bets for any attribute, while still being extendable by just adding another properties and attributes to the bets class. I also made other querying methods based on get_bets.

[amortization]

@skent259 I am second guessing our decision to make table an attribute of Player, and player and table an attribute of Bet. I think that what we have is a case of "circular references" which I have been reading about and think that it may be best if we avoid since this would allow Bets and Players to exist and have attributes independent of their table and player, and would allow easier testing, for example we can have one Bet object and throw a bunch of different tables at it to test for things without having to have a player actually make a bet on the table. I'm going to refactor it to remove these circular references and see how it looks. I think with the inheritance structure we have set up the code will be just as clean in most cases, and even cleaner in some cases.

[skent259]

@amortization I can see where the circular references could cause bugs and other issues down the line. It sounds like you have an idea of what that might look like, so go for it. Please let me know when to review again

[amortization]

@skent259 I removed the circular references. I think that this will be more maintainable moving forward.

[skent259]

Do you have any articles or things you were reading about circular references? I'm not convinced that the current approach is better than having table and player referenced in bet, but I'd like to get more knowledgeable on the topic. In particular, I'd like to understand when circular references are bad and what the major issues are in python.

These are the pros/cons that I see of the current approach that avoids the circular references

• Pro: Easier testing because we can apply multiple tables or players to bets
• Pro: Cleaner class variables
• Con: Some clunkiness in strategy.py. In particular, when placing odds we reference the player twice (perhaps allowing someone to use 2 different players by accident?) and creating a breaking change in player.bet() by adding the table every time.

I come back to the fact that a player should exist only on one table at a time. And their bets should also only exist on one table. So I'm not sure if that helps with the circular references or not

[amortization]

https://softwareengineering.stackexchange.com/questions/11856/whats-wrong-with-circular-references has some good info on why circular references are considered bad design. There are also a number of Python specific thing out there regarding how it can lead to memory issues since if Table is deleted but Bet still exists, Table is never actually deleted so then if Bet is deleted does Table still exist etc. I think newer versions of Python handle it better though. Python actually explicitly disallows circular imports of modules, that's why we have to put the typing.TYPE_CHECKING check at the beginning of bet.py to avoid a circular import error.

I don't think it's so much that Bet would exist on multiple tables, it's that Bet can exist independently from a table or player and still function with its own attributes independently without any Table or Player method existing.

I think a case could be made that Player can't or doesn't need to exist independently of Table, but then should Player be added to the Table module and only exist in the context of a Table since at that point Player really just exists as a dict inside of Table? I don't think so because I do think that there is room to grow with more player dependent attributes that should exist independently, but that's part of the reasoning.

Really it's probably more theoretical future proofing than anything right now. I do however want to add an equality check for Bet based on its attribute which will help with testing as we'll be able to do things like:

assert Odds6(12) in player.bets_on_table

Which would be much harder to do if player and table were attributes whose equality was being checked. That's why it's nice to have Bet be able to exist independently.

Functionally yeah the odds thing looks sorta silly having to reference player twice. The answer there could be to add place_odds and lay_odds convenience methods to Player so you can just call player.place_odds(6, table) to place odds on 6.

[amortization]

For a convenience method I added place_odds and lay_odds methods to player to place and lay odds without having to reference the bet and reference the player again.

For further convenience, I made it so that if no number is specified it would automatically check to see if only one of the given bets that allows odds exists and use that so you wouldn't necessarily have to know the number, for example in the case of PassLine. I also made it so that if no bet amount is specified, it will automatically assume that you want to place max odds.

[amortization]

I changed place_odds and lay_odds on player to a single method where the player can specify the types of bets they want to lay or place odds on.

[amortization]

Another place where I could see us using Bet as an object independent of Table and Player is in checking for Bet inside something. One could do something like:

player.has_bet(Odds6(25)) 

or

Odds6(25) not in player.bets_on_table

to check if the player has that bet without having to set the bet to that specific Table and Player.

[skent259]

I can definitely see bets existing independent of tables and players, but I worry about what it means when a player exists independent of a table.

Think about the following test:

def test_multi_table():
    player = Player(100)
    t1 = Table()
    t2 = Table()
    t1.add_player(player)
    t2.add_player(player)

    player.place_bet(PassLine(5), t1)
    player.place_bet(PassLine(5), t2)
    assert (player.bankroll, len(player.bets_on_table), player.total_bet_amount) == (90.0, 2, 10.0)

It fails because len(player.bets_on_table) is only 1. Even though t2 is specified in the second place bet, it adds to the bet on t1 anyways. I think really strange things like this are bound to show up when we don't follow some structure like a player can only sit at one table and place bets on the table it's sitting at. Both of those ideas support a reference within player to the table they are at.

Even with this reference to table, and after more reading, I don't think it has to be circular. In some sense, it's a parent-child relationship with the table being the parent. What do you think about using a weak reference to make this relationship explicit? There's some discussion here: https://pencilprogrammer.com/circular-reference-in-python/, and a deeper dive here on the topic: https://medium.com/@chipiga86/circular-references-without-memory-leaks-and-destruction-of-objects-in-python-43da57915b8d

This would still allow bets to exist independent, but we could change the player.place_bet() method to use the table object from the player implicitly. Players shouldn't be able to place bets if they aren't on a table, but the bets themselves can still exist.

Thoughts?

[amortization]

It was the inspect module that was adding time since it was getting the values of all of the attributes whether used or not. I redid it to no longer need that and it clocks in at 183ms which is pretty consistent with before, with a little more time added since more attributes are available. I still think that it may be worth relooking at those tests since getting random rolls can slow things down, and two tests will never be consistent. Without test_second_chunk it goes down to 7ms and without first chunk it goes down to <1ms. Also these tests aren't really verifying anything, it would be better off if they checked against some consistent set of data. For now it's probably worth just leaving it as it.

[amortization]

I left a print statement in there from debugging which I removed. Now the speed is back down to the 98ms which is consistent to what it was prior to the change.

[amortization]

I can definitely see bets existing independent of tables and players, but I worry about what it means when a player exists independent of a table.

Think about the following test:

def test_multi_table():
    player = Player(100)
    t1 = Table()
    t2 = Table()
    t1.add_player(player)
    t2.add_player(player)

    player.place_bet(PassLine(5), t1)
    player.place_bet(PassLine(5), t2)
    assert (player.bankroll, len(player.bets_on_table), player.total_bet_amount) == (90.0, 2, 10.0)

It fails because len(player.bets_on_table) is only 1. Even though t2 is specified in the second place bet, it adds to the bet on t1 anyways. I think really strange things like this are bound to show up when we don't follow some structure like a player can only sit at one table and place bets on the table it's sitting at. Both of those ideas support a reference within player to the table they are at.

Even with this reference to table, and after more reading, I don't think it has to be circular. In some sense, it's a parent-child relationship with the table being the parent. What do you think about using a weak reference to make this relationship explicit? There's some discussion here: https://pencilprogrammer.com/circular-reference-in-python/, and a deeper dive here on the topic: https://medium.com/@chipiga86/circular-references-without-memory-leaks-and-destruction-of-objects-in-python-43da57915b8d

This would still allow bets to exist independent, but we could change the player.place_bet() method to use the table object from the player implicitly. Players shouldn't be able to place bets if they aren't on a table, but the bets themselves can still exist.

Thoughts?

Table/Player I'm more comfortable with than Bet as long as we treat the Player as existing only in the context of Table a la a true Parent/Child relationship. I think that if Players should only exist in the context of a Table than we should require a table in the players init method and store it as a private variable only accessible through a property getter and setter method to enforce that table.add_player gets called. Something like:

class Player:
    def __init__(self, bankroll: typing.SupportsFloat,
                 table: Table,
                 bet_strategy: Strategy = BetPassLine(5),
                 name: str = "Player",
                 unit: typing.SupportsFloat = 5):
        ...
        self._table = None
        self.table = table

    @property
    def table(self):
        return self._table

    @table.setter
    def table(self, value: Table):
        value.add_player(self)
        self._table = value

I do still think that this is an antipattern but it would prevent a lot of the issues that could arise. I don't know enough about weakref to know the implications of implementing it, but it would probably make sense.

I think if we wanted to go down that route, we'd probably want to refactor all strategies to no longer require a table, as the table would just be taken from player.table to avoid sending a table that is not a part of the player.

[skent259]

Glad to hear the testing has dropped back down to normal levels. Given that these simulations can take a while already, I want to make sure we're not increasing it too much. I agree the tests would be better with a fixed set of rolls so that we can at least test the output. In fact, I think it would be helpful to do this on most strategies (with maybe 50-100 fixed rolls) to "lock-down" a bankroll value to make sure it doesn't change when we adjust strategy logic, especially if we implement something like #23

[skent259]

Table/Player I'm more comfortable with than Bet as long as we treat the Player as existing only in the context of Table a la a true Parent/Child relationship. I think that if Players should only exist in the context of a Table than we should require a table in the players init method and store it as a private variable only accessible through a property getter and setter method to enforce that table.add_player gets called. Something like:

class Player:
    def __init__(self, bankroll: typing.SupportsFloat,
                 table: Table,
                 bet_strategy: Strategy = BetPassLine(5),
                 name: str = "Player",
                 unit: typing.SupportsFloat = 5):
        ...
        self._table = None
        self.table = table

    @property
    def table(self):
        return self._table

    @table.setter
    def table(self, value: Table):
        value.add_player(self)
        self._table = value

I do still think that this is an antipattern but it would prevent a lot of the issues that could arise. I don't know enough about weakref to know the implications of implementing it, but it would probably make sense.

I think if we wanted to go down that route, we'd probably want to refactor all strategies to no longer require a table, as the table would just be taken from player.table to avoid sending a table that is not a part of the player.

As I understand, weakref mostly handles the appropriate garbage collection when objects get deleted. So if player has a weak reference to table, they don't get coupled together in memory.

I'm comfortable with what you proposed, but I want to make sure I understand. Would the following code still be possible?

table = Table()
table.add_player(Player(100))

Refactoring strategies to work only with the player in that way would be an improvement from my point of view, and probably clearer.

[skent259]

For the sake of clarity, can we merge this PR into the branch for #19 and then tackle the changes to the player module as described? I think the purpose of this PR has been completed.

[amortization]

Table/Player I'm more comfortable with than Bet as long as we treat the Player as existing only in the context of Table a la a true Parent/Child relationship. I think that if Players should only exist in the context of a Table than we should require a table in the players init method and store it as a private variable only accessible through a property getter and setter method to enforce that table.add_player gets called. Something like:

class Player:
    def __init__(self, bankroll: typing.SupportsFloat,
                 table: Table,
                 bet_strategy: Strategy = BetPassLine(5),
                 name: str = "Player",
                 unit: typing.SupportsFloat = 5):
        ...
        self._table = None
        self.table = table

    @property
    def table(self):
        return self._table

    @table.setter
    def table(self, value: Table):
        value.add_player(self)
        self._table = value

I do still think that this is an antipattern but it would prevent a lot of the issues that could arise. I don't know enough about weakref to know the implications of implementing it, but it would probably make sense. I think if we wanted to go down that route, we'd probably want to refactor all strategies to no longer require a table, as the table would just be taken from player.table to avoid sending a table that is not a part of the player.

As I understand, weakref mostly handles the appropriate garbage collection when objects get deleted. So if player has a weak reference to table, they don't get coupled together in memory.

I'm comfortable with what you proposed, but I want to make sure I understand. Would the following code still be possible?

table = Table()
table.add_player(Player(100))

Refactoring strategies to work only with the player in that way would be an improvement from my point of view, and probably clearer.

No, that wouldn't work as we would require a table when creating a player and having Player(100) exist independently of a table is what we want to avoid. A user could in the same amount of lines do:

table = Table()
player = Player(100, table)

[skent259]

No, that wouldn't work as we would require a table when creating a player and having Player(100) exist independently of a table is what we want to avoid. A user could in the same amount of lines do:

table = Table()
player = Player(100, table)

Presumably we could change table.add_player() to accept the player arguments though? Something like this within Table():

def add_player(self, **kwargs):
    player = Player(table = self, **kwargs)

Then

table = Table()
table.add_player(bankroll=100)

The other way wouldn't be bad, but this would be less of a breaking change and flow a bit better, IMO

[amortization]

I like that option better too as long as table is a required field of Player (and probably explicit argument for add player for documentation and simplicity.)