Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added display of probability distribution advice #38

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ka-weng1
Copy link
Collaborator

@ka-weng1 ka-weng1 commented Jan 8, 2025

Added display of the probability distribution of orders predicted by the logistic regression model when user holds the shift key and selects provinces. Based on configuration (see section Display Configuration), it either visually renders the distribution directly on the map or through textual messages in the move advice box.

Running the code

  • To run the code, the model path to the logistic regression model must be specified in the diplomacy/utils/models.py file.
  • the requirements.txt file has also been changed to include the baseline_models package so model utils functions can be used.

Display Configuration

When you click Create Game in the UI, in the other settings page of game creation form, there is a selection for setting the display of the distribution advice with options being (no advice), visual and textual.

Visual Advice

To get advice, press and hold shift key, and click on provinces you would like to get advice for. Multiple provinces may be selected to view their advice simultaneously.

When holding down the shift key, you may create an order for an orderable province by clicking on the province once again to start order building.

When shift key is lifted, all advice will be cleared.

Textual Advice

To get advice, press and hold shift key, and click on provinces you would like to get advice for. Textual messages containing advice orders and their corresponding probability will be shown.

Two buttons Accept and Show are shown next to each advice order. By clicking on Show, you will able to display the order on map persistently until the shift key is lifted. By clicking on Hide, you will able to hide the order when the shift key is still being held. By clicking on Accept, you will be able to build the advice order you have accepted.

When shift key is lifted, all advice will be cleared.

Copy link

@aphedges aphedges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very sorry for taking so long to get to reviewing this! The PR is very large, and the size was daunting. I should not have procrastinated my review this long. I hope that we can work together over the next several days to get this PR merged before the PI meeting.

You made a large number of changes, and I am unfamiliar with much of the original code you modified, so in the interest of time, I'm not going to do a full review of all of your code. It's taking too long already, so I will instead focus on making sure your changes work and don't break anything. We can always make further improvements after the PR is merged.

Here are the initial steps you should take so I can properly test out your code:

  • Delete common_old/ and SvgStandard.old.js. These are clearly old files that aren't needed and make the PR much larger than it needs to be.
  • Rebase your branch against main to fix the conflicts. Squashing some of your commits first might help with this process and also make your changes easier to review. I don't know about your experience with Git, but if you need help, please let me know.
  • Once Migrate from setup.py to pyproject.toml #46 is reviewed and merged, rebase against main again and switch to installing baseline_models as an optional dependency. You basically just need to copy the code from lines 43-45 of pyproject.toml in chiron-utils into pyproject.toml in this repository. Make sure you adjust the commit when doing so.
  • I need to make some changes so your code will still work when we run the server with Docker and when diplomacy is used by other bots. Once you've made the changes I've described above, I'll push the needed commits to your branch.

On Slack, you said:

I have some implementation details I would like to cross check with you: currently, I am running the model on the server and was handling the client server communication myself. But I have noticed that for other advices implemented, a bot is connected to port to provide advice. Should I modify my current implementation to be aligned and if yes, how can I modify that?

For now, making baseline_models an optional dependency is a good enough solution. The logistic regressor model is very lightweight, so there is no problem including it directly. However, if a larger or more complex model is needed (e.g., one that requires a GPU), then we will need to figure out how to move this kind of logic into a bot. If we encounter that problem later, we can change the architecture then.

@ka-weng1 ka-weng1 force-pushed the distribution-advice branch from 3accee8 to 13a36f1 Compare February 16, 2025 04:41
@ka-weng1 ka-weng1 force-pushed the distribution-advice branch from 13a36f1 to c913d8f Compare February 16, 2025 05:42
@ka-weng1 ka-weng1 force-pushed the distribution-advice branch from dafddb3 to 51ec2ee Compare February 16, 2025 08:57
@ka-weng1
Copy link
Collaborator Author

Hi Alex, thank you for reviewing this PR!

I have made the following changes:

  • deleted common_old/ and SvgStandard.old.js
  • squashed my commits
  • rebased against main
  • moved the baseline-models requirement as optional requirement into pyproject.toml.
    However the prediction logic is currently stored under a branch in baseline-models (see PR baseline-lr distribution advice for game engine baseline-models#5), and the diplomacy dependency for baseline-models conflicts with the current diplomacy version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants