UniversalPoker/ACPC fixes: fullgame+abstracted MaxGameLength, fullgame LegalActions

[VitamintK]

A pull request with 3 small fixes for UniversalPoker/ACPC

1. MaxGameLength for fullgame (i.e. unabstracted game):
   MaxGameLength previously was written only for abstracted games where the only bet size is pot-size, so it assumed that each successive bet had to double in size. This adds a fix to MaxGameLength for fullgame and FCHPA (where a half-pot bet is legal) where we assume only that each bet has to be greater than the size of the biggest blind. This fixes MaxGameLength for fullgame and FCHPA, as well as simplifying the MaxGameLength logic for all abstractions. The size of InformationStateTensor is based on MaxGameLength, so this fixes InformationStateTensors.
2. MaxGameLength for abstracted game:
   Change length += NumPlayers() to length += NumPlayers() - 1 for a tighter estimate for abstracted games.
3. LegalActions for fullgame
   In unabstracted games, when a hand goes to showdown, LegalActions still returned a set of raises, despite the state being terminal. This is a fix for that.

[lanctot]

Thanks @VitamintK !

The tests failed because the playthrough dors not match. Can you regenerate the playthrough and add it to the PR?

[VitamintK]

Done. Also added a test to universal_poker_test.cc and verified that it fails without the changes in this pull request.

[jhtschultz]

Why is it n-2 players call and not n-1?

[VitamintK]

Good question. Here's my thought process. Also let me know if you see anything wrong with the reasoning.

The longest game (with a single betting round, for simplicity) consists of:

n-1 players checking,
1 player betting, n-2 players calling,
1 player raising, n-2 players calling,
etc...,
1 player raising, n-1 players calling

so each min-bet/min-raise is succeeded by n-2 calls (except for the last bet, which is succeeded by n-1). In addition, there are n-1 checks in the beginning. These n-1 checks + the final 1 call add up to n, which is accounted for a few lines above in the code (// Check Actions).

[VitamintK]

Actually it's probably convenient for future readers of the code if I just put this comment as a code comment. What do ya think?

[jhtschultz]

Thanks for the clarification. This reasoning makes sense, but there's still a few points to resolve.

First, the calculation on line 1101 only accounts for the number of raises, not all of the n-2 calls in between. So I believe that has to be updated.

Second, maybe there's a simpler way to structure this that better handles all betting abstractions. The goal is to figure out what the max number of raises is and then apply your logic regarding max number of check/calls.

max_num_raises = 0
if betting_abstraction == kFC:
  pass  # no raises allowed
elif betting_abstraction == kFCPA:
  pot_size = maxBlind * num_players
  while pot_size / num_players < maxStack:
    max_num_raises += 1
    pot_size += pot_size * num_players
elif betting_abstraction == kFCHPA:
  pot_size = maxBlind * num_players
  while pot_size / num_players < maxStack:
    max_num_raises += 1
    pot_size += pot_size / 2 * num_players
elif betting_abstraction == fullgame:
  max_num_raises = (maxStack+maxBlind-1)/maxBlind

length += max_num_raises * (num_players - 1)

I think this is more intuitive than dividing the max stack size in half. I totally might be missing something on either of these points though so let me know what you think. Funny how even these simple things can be tricky!

[VitamintK]

First, the calculation on line 1101 only accounts for the number of raises, not all of the n-2 calls in between. So I believe that has to be updated.

ah yes, thanks. 😅 good catch!

I think this is more intuitive than dividing the max stack size in half.

Yup, definitely. The logic in your snippet all looks good, thanks!

Funny how even these simple things can be tricky!

On the bright side, this has finally made me learn how to calculate a pot-sized bet.

[jhtschultz]

Looks good, just left a comment with one question. And thanks for adding the test!

One other thing, please run it through a linter (see recently added bullet 8 from https://github.com/deepmind/open_spiel/blob/master/docs/developer_guide.md#adding-a-game for links to linters) since there's a few style guide things I think the linter will pick up on. Thanks

[VitamintK]

I ran downloaded cpplint.py and ran python cpplint.py open_spiel/games/universal_poker.cc and there was no output. Does that mean it passed the linter?

[jhtschultz]

Hmm I'd be surprised if there were no output. Did you just download the file or pip install cpplint? That could be it; my understanding is that you're supposed to run it as a command line tool.

[VitamintK]

Ah, I figured out the problem. The github.com/google/styleguide cpplint is mostly unmaintained and requires python 2, and fails silently when ran with python 3 😑: google/styleguide#132

There's a fork (which is what you get from pip install cpplint) that works with python3, so I'll run that.

As per google/styleguide#528 (comment) it's not clear whether the google/styleguide cpplint will be maintained going forward, so you might want to change the link in bullet 8 to the pip install cpplint fork (maybe this link?), to prevent future contributors from getting the silent failure.

[jhtschultz]

Ahh thanks for catching that. Yeah good call I just sent out a CL to update the link in the dev guide.

[jhtschultz]

LGTM