Conversation
👷 Deploy request for catanatron-staging pending review.Visit the deploys page to approve it
|
d6c285f to
1284789
Compare
ba3b429 to
9ee99a7
Compare
bcollazo
left a comment
There was a problem hiding this comment.
Ok, pretty cool! Thanks for opening this PR!
Please don't be discouraged if we go back and forth a bit on the PR. I want to make sure I understand it fully and it goes in the direction I want for the codebase.
Could you include a video demo or so of the feature at work? Also explain a bit more how it may work? Maybe add a couple more examples like test_discard_possibilities_are_per_resource. Its a good example, but a simple one. I'd like to see a couple more to fully understand the picture. Thanks!
|
|
||
| # TODO: None for now to avoid complexity, but should be Resource[]. | ||
| DISCARD = "DISCARD" # value is None | ||
| DISCARD = "DISCARD" # value is Resource |
There was a problem hiding this comment.
Can you rename the action all together to DISCARD_RESOURCE? I think its going to help discern from a regular DISCARD.
There was a problem hiding this comment.
did a grep search for every file where this action type is found and made the change
| abort(404) | ||
| db.session.commit() | ||
| game = pickle.loads(result.pickle_data) # type: ignore | ||
| game.state._state_index = result.state_index |
| self.state = State(players, catan_map, discard_limit=discard_limit) | ||
| self.playable_actions = generate_playable_actions(self.state) | ||
|
|
||
| def __setstate__(self, state): |
|
|
||
| def __setstate__(self, state): | ||
| self.__dict__ = state | ||
| if not hasattr(self, "action_records"): |
There was a problem hiding this comment.
I think I see what we are trying to do here. Save old games? I rather have the simplicity in the code and have users treat games ephemerally (or tied to a version of the codebase).
There was a problem hiding this comment.
was a patch for a database issue with the --step-db CLI flag that has since been resolved. Going to remove these above 3 legacy functions
| assert action.value == (SHEEP,) | ||
|
|
||
|
|
||
| def test_action_from_json_discard(): |
|
Also, be sure to rebase and address any and all CI issues. Really want this to get in; I think its a strict improvement to what we have in place, and its pretty much needed for the UI to be usable. |
69f97b0 to
68722f0
Compare
|
Also, rebase or repoint the PR against bcollazo:main! master no longer! 👍 Thanks. |
should be pointing to main with this rebase: 99657b3. Let me know if I'm mistaken |
bcollazo
left a comment
There was a problem hiding this comment.
Hey, thanks for the work here. I have some changes I'd like to make before we merge. Please take a look! Thanks!
| # Preserve historical DISCARD ordering so the rename does not reshuffle | ||
| # integer action ids for gym consumers. | ||
| return str(action).replace("DISCARD_RESOURCE", "DISCARD") | ||
|
|
There was a problem hiding this comment.
Is this for backwards compatibility as well? I wouldn't invest in it in the repo. I rather have it simple and treat it as a breaking change.
There was a problem hiding this comment.
yup, this was for a deep learning bot in another repo trained on the 290 sized action space to work with the new 294 sized action space. Can remove this for the main repo though
| def discard_possibilities(state: State, color) -> List[Action]: | ||
| if state.discard_counts[color] <= 0: | ||
| return [] | ||
|
|
||
| return [ | ||
| Action(color, ActionType.DISCARD_RESOURCE, resource) | ||
| for resource in RESOURCES | ||
| if player_num_resource_cards(state, color, resource) > 0 | ||
| ] |
| The "result" field is polymorphic depending on the action_type. | ||
| - ROLL: result is (int, int) 2 dice rolled | ||
| - DISCARD: result is List[Resource] discarded | ||
| - DISCARD_RESOURCE: result is List[Resource] discarded in this action |
| def normalize_discarded_cards(state: State, action: Action, action_record=None): | ||
| if action.value is not None: | ||
| if isinstance(action.value, (list, tuple)): | ||
| return list(action.value) | ||
| return [action.value] | ||
|
|
||
| if action_record is not None and action_record.result is not None: | ||
| if isinstance(action_record.result, (list, tuple)): | ||
| return list(action_record.result) | ||
| return [action_record.result] | ||
|
|
||
| hand = player_deck_to_array(state, action.color) | ||
| num_to_discard = len(hand) // 2 | ||
| if action_record is None: | ||
| # TODO: Forcefully discard randomly so that decision tree doesnt explode in possibilities. | ||
| discarded = random.sample(hand, k=num_to_discard) | ||
| else: | ||
| discarded = action_record.result # for replay functionality | ||
| return [random.choice(hand)] |
There was a problem hiding this comment.
What's the purpose of this function? Not following. Can we simplify and have the .value of DISCARD_RESOURCE be a resource and that's it? Not a list.
There was a problem hiding this comment.
Also, why would we have to random.choice(hand) here? I think with this solution of discarding one at a time, we wouldn't need to random choose here, no?
| if isinstance(value, list): | ||
| if len(value) != 1: | ||
| raise ValueError( | ||
| "Discard action must have 1 resource when encoded as a list" | ||
| ) | ||
| value = value[0] |
There was a problem hiding this comment.
Same here. Sounds like simplifying the .value to always a resource would simplify this code too!
| self.discard_counts: Dict[Color, int] = {color: 0 for color in self.colors} | ||
| self.discard_counts: Dict[Color, int] = {color: 0 for color in self.colors} |
| def test_discard_possibilities_are_per_resource(): | ||
| player = SimplePlayer(Color.RED) | ||
| state = State([player]) | ||
| state.discard_counts[player.color] = 2 | ||
|
|
||
| player_deck_replenish(state, player.color, WHEAT, 2) | ||
| player_deck_replenish(state, player.color, BRICK, 1) | ||
|
|
||
| assert discard_possibilities(state, player.color) == [ | ||
| Action(player.color, ActionType.DISCARD_RESOURCE, BRICK), | ||
| Action(player.color, ActionType.DISCARD_RESOURCE, WHEAT), | ||
| ] |
There was a problem hiding this comment.
Thank you! Can you add a couple more tests of this nature?
| } from "./api.types"; | ||
| import type { GameState } from "./api.types"; | ||
|
|
||
| export function humanizeAction(gameState: GameState, action: GameAction) { |
There was a problem hiding this comment.
Hmm.. we shouldn't need this function. Everything in the log should be ActionRecords.
|
Ahh, finally the checks were able to run. I think it may have been a transient error on Github's part? Anyways, let me know if you have any questions about CI checks and how to make them all green. 👍 |
Deterministic Discards
Pull Request Test Coverage Report for Build 23776692959Details
💛 - Coveralls |
|
Awesome. Thank you so much for taking on this work! Makes it a lot more usable and representative. 👍 |

implemented deterministic discards and updated the web UI to allow for resources to be selected to be discarded one by one.
#361