-
Notifications
You must be signed in to change notification settings - Fork 264
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 docstrings to strategy methods #1375
Conversation
caddycarine
commented
Sep 26, 2020
- Added docstrings to strategy methods so they don't display the default placeholder docstring from the Player class.
- Corrected (comments) typos in player, tournament, and _strategy_utils
- Extracted calculation of cooperation ratio into a new method in averagecopier.
…t placeholder docstring from the Player class. Corrected (comments) typos in player, tournament, and _strategy_utils Extracted calculation of cooperation ratio into a new method in averagecopier.
Thanks for this @caddycarine. I believe the test are failing because of a small error in the docs that was fixed in #1372. Specifically in
should be:
(There's actually two spots where that occurs). There are two fixes here:
|
* Add check to config.yml * Install black before trying to use it. * Move black installation This is just to avoid installing in case the CI fails beforehand. * Run black * Additional documentation on running black * Revert "Run black" This reverts commit c50a6e3. * Remove unnecessary installs. * Revert "Revert "Run black"" This reverts commit 10ab222. * Run isort. * Make black and isort compatible. Co-authored-by: T.J. Gaffney <[email protected]>
* Write a new documentation page with branch info This is related to #1352. Once this PR is merged we should: - Change github default branch to `dev` - Delete `master` - Confirm that read the docs is looking at `release` Am I missing anything? We should also remove the release process information from the wiki (assuming we're happy with what I've written here). Finally, once #1360 is done we should make sure we update the docs with the relevant information. * Remove ambiguous `very`. * Remove hypothesis version specification in docs. This is actually no longer correct since #1288 * Test properties not affected by floating point error This build found a particular failing example of `TestTournament.test_seeding_equality` https://github.com/Axelrod-Python/Axelrod/pull/1368/checks?check_run_id=975415322 Upon closer investigation it looks like that was not due to seeding but due to the floating point error of some calculations made by the result set. I investigated using: ``` import axelrod as axl import numpy as np seed = 2 repetitions = 10 rng = axl.RandomGenerator(seed=seed) players = [axl.Random(rng.random()) for _ in range(8)] tournament1 = axl.Tournament( players=players, turns=10, repetitions=repetitions, seed=seed ) tournament2 = axl.Tournament( players=players, turns=10, repetitions=repetitions, seed=seed ) for _ in range(4): results1 = tournament1.play(processes=2, progress_bar=False) results2 = tournament2.play(processes=2, progress_bar=False) assert results1.wins == results2.wins assert results1.match_lengths == results2.match_lengths assert results1.scores == results2.scores assert np.allclose(results1.normalised_scores, results2.normalised_scores) assert np.allclose(results1.ranking, results2.ranking) assert results1.ranked_names == results2.ranked_names assert results1.payoffs == results2.payoffs assert results1.payoff_matrix == results2.payoff_matrix assert np.allclose(results1.payoff_stddevs, results2.payoff_stddevs) assert results1.score_diffs == results2.score_diffs assert results1.payoff_diffs_means == results2.payoff_diffs_means assert results1.cooperation == results2.cooperation assert results1.normalised_cooperation == results2.normalised_cooperation assert results1.vengeful_cooperation == results2.vengeful_cooperation assert results1.cooperating_rating == results2.cooperating_rating assert results1.good_partner_matrix == results2.good_partner_matrix assert results1.good_partner_rating == results2.good_partner_rating assert np.allclose(results1.eigenmoses_rating, results2.eigenmoses_rating) assert np.allclose(results1.eigenjesus_rating, results2.eigenjesus_rating) ``` Note I'm using `np.isclose` for some properties. In this commit: - I add the specific seed for which the error was found as a hypothesis example (`seed=2`). - Replace the `results1 == results2` check with just a check of some properties (from which the others are essentially calculated). - Added `progress_bar=False` * Add instructions for using venv. * s/requirements/requirements.txt * Spell requirements correctly..
* Added new SpitefulCC Player to grudger.py, updated the list of all strategies and added coverage test in test_grudger.py * changed 236 strategies to 237 in docs/index.rst * changed SpitefulCC classifier and added strategy description * changed description spitefulCC strategy * Fix doctest. This was failing on #1364 but the fix is trivial. * Correct expected actions for meta strategy. * Run black and isort. Co-authored-by: Laura Romero <[email protected]>
* [#1370] Remove some tests from test_meta.py Removes some tests that do not target specific strategy behaviors and can change when new strategies are added. * Add a property based test for valid strategies Not sure if this is overkill @marcharper, just thought it could be worth having. * Run isort. Co-authored-by: Vince Knight <[email protected]>
…t placeholder docstring from the Player class. Corrected (comments) typos in player, tournament, and _strategy_utils Extracted calculation of cooperation ratio into a new method in averagecopier.
…rategies and added coverage test in test_grudger.py
change line 641 of axelrod/tests/strategies/test_meta.py
The CI is failing on
Could you run:
at the root of the repo (should modify those 3 files). |
* Add check to config.yml * Install black before trying to use it. * Move black installation This is just to avoid installing in case the CI fails beforehand. * Run black * Additional documentation on running black * Revert "Run black" This reverts commit c50a6e3. * Remove unnecessary installs. * Revert "Revert "Run black"" This reverts commit 10ab222. * Run isort. * Make black and isort compatible. Co-authored-by: T.J. Gaffney <[email protected]> Rebase with dev branch Resolved merge conflicts in last commit
* Add check to config.yml * Install black before trying to use it. * Move black installation This is just to avoid installing in case the CI fails beforehand. * Run black * Additional documentation on running black * Revert "Run black" This reverts commit c50a6e3. * Remove unnecessary installs. * Revert "Revert "Run black"" This reverts commit 10ab222. * Run isort. * Make black and isort compatible. Co-authored-by: T.J. Gaffney <[email protected]> Rebased to dev branch Resolved merge conflicts Rebase with dev branch
There seems to be a merge conflict in |
Rebase with main
* Add check to config.yml * Install black before trying to use it. * Move black installation This is just to avoid installing in case the CI fails beforehand. * Run black * Additional documentation on running black * Revert "Run black" This reverts commit c50a6e3. * Remove unnecessary installs. * Revert "Revert "Run black"" This reverts commit 10ab222. * Run isort. * Make black and isort compatible. Co-authored-by: T.J. Gaffney <[email protected]> Rebase with main Rebase with master
* Added new SpitefulCC Player to grudger.py, updated the list of all strategies and added coverage test in test_grudger.py * changed 236 strategies to 237 in docs/index.rst * changed SpitefulCC classifier and added strategy description * changed description spitefulCC strategy * Fix doctest. This was failing on #1364 but the fix is trivial. * Correct expected actions for meta strategy. * Run black and isort. Co-authored-by: Laura Romero <[email protected]>
Resolved merge conflicts Modified for loop to stop initialization the filterset dictionary and comprehension set in each iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you @caddycarine.
Why are so many files affected? I thought we ran black recently. |
We did so I'm not too sure but I think this is because of how the commits were applied. Looks like a commit from the "applying black" PR 87fa66e was perhaps cherry picked which added merge conflicts and then black was run again. So my guess is that a local rebase would clean up the history. It could be worth:
If I'm reading everything correctly the only commit that's actually needed in this PR is |
Changed the test to make it use hypothesis to generate a list of classifiers to be used in the filters. Also made some minor modifications in some strategies' tests, like changing set(["game"]) to {"game"} to reduce function call.
Changed the test to make it use hypothesis to generate a list of classifiers to be used in the filters. Also made some minor modifications in some strategies' tests, like changing set(["game"]) to {"game"} to reduce function call.
@caddycarine After you follow the commands above, you'll need to open a new PR from the add_docstrings branch |
Changed the test to make it use hypothesis to generate a list of classifiers to be used in the filters. Also made some minor modifications in some strategies' tests, like changing set(["game"]) to {"game"} to reduce function call. Formatted with black and isort
After following the commands, do i rebase onto master before opening the PR? |
The second command |
Closing this as #1377 has been merged. |