poke-env icon indicating copy to clipboard operation
poke-env copied to clipboard

A whole bunch of useful improvements

Open cameronangliss opened this issue 1 year ago • 6 comments

This PR is a bag of miscellaneous improvements, including:

  1. A new Player method called battle_against_multi which allows you to play one Player instance against a list of Player instances without having to play everyone against everyone as is done in cross_evaluate. Also, cross_evaluate is simplified by using battle_against_multi internally.
  2. Some methods of Player are made static that don't need the self value, allowing users of poke-env to use those methods in their own static methods.
  3. accept_open_team_sheets is possible to turn on for OpenAIGymEnv instances as well as Player instances now.
  4. The team being used by a Player instance is now re-rolled at the beginning of every game played if a TeamBuilder is specified.
  5. There is no longer a timeout for connecting the websocket in the Showdown client.
  6. A small bug in the name of the effect TYPECHANGE was fixed.
  7. A bug in the choose_move method of SimpleHeuristicsPlayer was fixed where a decision wouldn't be made if the active pokemon on either side is None.

cameronangliss avatar Jun 26 '24 09:06 cameronangliss

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.46%. Comparing base (f458350) to head (f257d09). Report is 104 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   83.38%   85.46%   +2.07%     
==========================================
  Files          39       42       +3     
  Lines        3918     4423     +505     
==========================================
+ Hits         3267     3780     +513     
+ Misses        651      643       -8     

codecov[bot] avatar Jun 26 '24 10:06 codecov[bot]

@hsahovic this is ready to go whenever you're ready to take it!

cameronangliss avatar Jul 15 '24 00:07 cameronangliss

Just as a note, I could be doing reset_battles on self and opponent in the _battle_against method, and that's probably the thing that makes most sense given that the function now returns the win and lose rates, but I didn't want to make a compatibility issue with the previous functionality. @hsahovic let me know if you care about compatibility with this method or not, and if not I can put reset_battles in there. Currently it's in _battle_against_multi.

cameronangliss avatar Jul 15 '24 03:07 cameronangliss

After using this diff myself for a little while, I decided that it would be better to just to reset_battles in the battle_against method. I just put that change in.

cameronangliss avatar Jul 20 '24 23:07 cameronangliss

I agree that the current way results are handled by player objects can be improved, but i don't think starting battles should erase the previous results. These two actions should be covered by different methods in the API. I'm open to a proposal for a better structure around it, but i'm not sure this is it. Let's chat on discord? Your PR seems to contain other changes - do you want to open separate PRs for them?

hsahovic avatar Aug 12 '24 23:08 hsahovic

@hsahovic sorry, I've been MIA for a little while, but I've cleaned this PR up and it's ready for review :)

cameronangliss avatar Sep 29 '24 20:09 cameronangliss

Hi @hsahovic, I just wanted to check in on how you're feeling about this PR. It's a bit smaller now that some overlapping changes from @caymansimpson have been merged! :)

I've just updated the description, so it should be fully up-to-date with the diff. Please let me know if you would like to discuss any particular element of the changes. I'm just leaving them all clumped together because I've been too busy to get to splitting it up, but if this is a big problem then we could just wait to put this stuff in until I have the time to break it up. Hopefully I can convince you that this is all sound though :)

cameronangliss avatar Nov 21 '24 08:11 cameronangliss

Hey @cameronangliss, I missed your comments - sorry for the delay. Thanks for this PR and good update.

A new Player method called battle_against_multi which allows you to play one Player instance against a list of Player instances without having to play everyone against everyone as is done in cross_evaluate. Also, cross_evaluate is simplified by using battle_against_multi internally.

I don't think this is a good API - however, making battle_against accept multiple opponents (either in *args or with an iterable) would probably be a good change, if you want to do so.

Some methods of Player are made static that don't need the self value, allowing users of poke-env to use those methods in their own static methods. accept_open_team_sheets is possible to turn on for OpenAIGymEnv instances as well as Player instances now. The team being used by a Player instance is now re-rolled at the beginning of every game played if a TeamBuilder is specified. A bug in the choose_move method of SimpleHeuristicsPlayer was fixed where a decision wouldn't be made if the active pokemon on either side is None.

These are good changes, i'd be happy to merge them

There is no longer a timeout for connecting the websocket in the Showdown client.

I would be ok with an optional parameter with a reasonable default value that can be set to infinity with a 0 or None, but no timeout by default is a gotcha in my opinion.

If you want to update this PR for these changes I'd be happy to review it - otherwise if you prefer we can also do one PR per change. Let me know what you think!

hsahovic avatar Nov 24 '24 21:11 hsahovic