Refactor and add `num_nodes` and `verbose` options to `get_top_moves()` #16

knutole · 2023-04-07T22:01:15Z

This PR combines and replaces #14 and #15 , with major refactor of get_top_moves() function. (It was much easier to combine the PR's, for obvious reasons.) I have left in quite a few comments in the refactor, to make it easier to follow. I will remove most of them once you've had a chance to look it over.

Refactor get_top_moves(), remove redundant code
Add support for 'num_nodes', see Add support for 'go nodes' in get_top_moves #14
Add support for 'verbose', see Add include_info=True flag to .get_top_moves() #15
Add tests for new arguments
Rename 'N/s' to 'NodesPerSecond'
Add turn_perspective global option, defaults to Turn perspective. (Note: this means that get_top_moves() will change from returning White perspective to returning Turn perspective by default, which is not backwards compatible. See Ensure consistent evaluation perspective across the code base #18)
Add get/set for self._turn_perspective
Add get/set for self._num_nodes
Add self._depth as int and add public getter get_depth(). Fixes Improving some small things about self.depth #8
Add type checks for set_depth(), set_num_nodes(), set_turn_perspective()
Add more tests for instantiating Stockfish class
Add test for _set_option(). Remove redundant default update_parameters_attribute arguments.
Add convenience method _pick() to avoid doing current_line[current_line.index("cp") + 1]

Todo: Add turn_perspective option for rest of codebase, see #18

lint with black add dict type more elaborate typing type type this ignore type fixed Separete moves, info in Tuple if include_info=True lint with black add test revert; add test fix test cleanup

Add suggested cleanup Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

- Add support for 'num_nodes', see #14 - Add suppoert for 'verbose', see #15 - Refactor get_top_moves, remove redundant code - Add tests for new arguments - Rename 'N/s' to 'NodesPerSecond' - Add turn_perspective global option - Add get/set for self._num_nodes - Add get/set for self._turn_perspective - Todo: Refactor turn_perspective option for rest of codebase

github-actions · 2023-04-07T22:17:23Z

Coverage report

The coverage rate went from 93.35% to 94.78% ⬆️
The branch rate is 88%.

96.72% of new lines are covered.

Diff Coverage details (click to unfold)

stockfish/models.py

96.72% of new lines are covered (94.78% of the complete file).
Missing lines: 660, 664

- Fixes #8

…ensure type checking

kieferro · 2023-04-08T22:53:15Z

Wow, that's quite a PR. First of all, a big thank you for the work and the interest that you put in this project!
I am currently not able to test and review your code, but it looks like you're still at work anyway. So just ping us or request if you need a review.

knutole · 2023-04-10T08:21:38Z

It's not possible for me to request review on this PR, by the way, not sure if there's a Github setting for that. @kieferro @johndoknjas

kieferro

Okay, I am back and I looked at the changes. It looks very good overall!
We should, as you also wrote in your comment, implement the use of turn_perspective more consistently, but that's probably beyond the scope of this PR. This is definitely a good first step and we'll continue it from there.

stockfish/models.py

tests/stockfish/test_models.py

stockfish/models.py

kieferro · 2023-04-15T00:02:57Z

It's not possible for me to request review on this PR, by the way, not sure if there's a Github setting for that. @kieferro @johndoknjas

Ah okay, I see. But it seems that nothing can be done about that as long as you don't have write-access. But feel free to just use the draft PR feature to indicate when the PR is ready for review.

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

stockfish/models.py

README.md

tests/stockfish/test_models.py

kieferro

LGTM

johndoknjas

The changes look good, and the tests pass for me - nice job! Thanks for your contribution.

johndoknjas · 2023-04-20T05:12:06Z

Looks like we're all satisfied with the PR, so I've merged it into master.

knutole · 2023-04-20T05:16:06Z

Great, thanks 🙏👍

kieferro · 2023-04-20T19:10:04Z

Looks like we're all satisfied with the PR, so I've merged it into master.

Thanks 👍

Congratulations @knutole. Could you close your two PRs in the old repo (zhelyabuzhsky#115, zhelyabuzhsky#116)? This would give us a better overview.

knutole and others added 5 commits September 29, 2022 15:42

Add include_info=True flag to .get_top_moves()

8a5ebdc

lint with black add dict type more elaborate typing type type this ignore type fixed Separete moves, info in Tuple if include_info=True lint with black add test revert; add test fix test cleanup

Update stockfish/models.py

c201434

Add suggested cleanup Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

Update readme with include_info flag

0906d2e

Update README. Cleanup.

53e23e8

knutole changed the title ~~Improve get top moves~~ Improve get_top_moves(): Add "num_nodes" and "verbose" options. Refactor. Apr 7, 2023

Merge branch 'master' into improve_get_top_moves

0b0eadb

knutole mentioned this pull request Apr 7, 2023

Add include_info=True flag to .get_top_moves() #15

Closed

lint with black

aad00a5

knutole mentioned this pull request Apr 7, 2023

Add support for 'go nodes' in get_top_moves #14

Closed

knutole added 8 commits April 8, 2023 00:32

Rename to is_turn_perspective; cleanup

666113f

Remove unnecessary depth arg

cb0750f

Remove tests for get_top_moves(depth)

b81106b

Update tests

5650fab

Use self._depth as int, add public getter

29e2f33

- Fixes #8

Add type check for .set_depth(); add tests

48a3725

Add type check for .set_num_nodes(); add tests

99efaa4

Add type check for .set_is_turn_perspective(); add tests

aa07dbd

knutole mentioned this pull request Apr 8, 2023

Ensure consistent evaluation perspective across the code base #18

Open

knutole added 7 commits April 8, 2023 23:14

Add types to new options

8e34653

Use public functions to set depth, num_nodes, is_turn_perspective to …

4dc39c2

…ensure type checking

Fix grammar

1399db7

Add tests for instantiating Stockfish class

2663e6f

Add test coverage for some missing lines

c95a88e

Cleanup test names

bf8af50

Fix minor cosmetic issues

6513973

knutole changed the title ~~Improve get_top_moves(): Add "num_nodes" and "verbose" options. Refactor.~~ Refactor and add "num_nodes" and "verbose" options to .get_top_moves() Apr 8, 2023

Rename is_turn_perspective -> get/set turn_perspective

6b09ab4

knutole changed the title ~~Refactor and add "num_nodes" and "verbose" options to .get_top_moves()~~ Refactor and add num_nodes and verbose options to get_top_moves() Apr 10, 2023

Add convenience method _pick()

61e00e7

kieferro requested changes Apr 14, 2023

View reviewed changes

kieferro requested a review from johndoknjas April 15, 2023 00:00

knutole and others added 6 commits April 16, 2023 02:12

Split tests

61288e9

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

Split tests

89690fc

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

Remove default list in _pick

94ad50d

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

Fix typo

ee945dc

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

Fix typo

22cc3e3

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

Fix typo

996afa2

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>

knutole commented Apr 16, 2023

View reviewed changes

stockfish/models.py Outdated Show resolved Hide resolved

knutole and others added 3 commits April 16, 2023 02:16

Update text

8c9e95e

Lint

45f50ac

Lint with black

028e4da

knutole requested a review from kieferro April 16, 2023 00:38

kieferro approved these changes Apr 17, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

tests/stockfish/test_models.py Outdated Show resolved Hide resolved

knutole added 4 commits April 17, 2023 13:08

Review: update README, parametrize tests

3fdfec4

Fix tests

fcc198a

Remove comments

a33579d

Fix minor formatting errors

e4f261d

kieferro approved these changes Apr 17, 2023

View reviewed changes

johndoknjas approved these changes Apr 20, 2023

View reviewed changes

johndoknjas merged commit e2f8c4c into py-stockfish:master Apr 20, 2023

This was referenced Apr 20, 2023

Option to return more info from Stockfish in .get_top_moves() zhelyabuzhsky/stockfish#115

Closed

Add support for 'go nodes' in get_top_moves zhelyabuzhsky/stockfish#116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and add `num_nodes` and `verbose` options to `get_top_moves()` #16

Refactor and add `num_nodes` and `verbose` options to `get_top_moves()` #16

knutole commented Apr 7, 2023 •

edited

Loading

github-actions bot commented Apr 7, 2023 •

edited

Loading

stockfish/models.py

kieferro commented Apr 8, 2023

knutole commented Apr 10, 2023

kieferro left a comment

kieferro commented Apr 15, 2023

kieferro left a comment

johndoknjas left a comment

johndoknjas commented Apr 20, 2023

knutole commented Apr 20, 2023

kieferro commented Apr 20, 2023

Refactor and add num_nodes and verbose options to get_top_moves() #16

Refactor and add num_nodes and verbose options to get_top_moves() #16

Conversation

knutole commented Apr 7, 2023 • edited Loading

github-actions bot commented Apr 7, 2023 • edited Loading

Coverage report

stockfish/models.py

kieferro commented Apr 8, 2023

knutole commented Apr 10, 2023

kieferro left a comment

Choose a reason for hiding this comment

kieferro commented Apr 15, 2023

kieferro left a comment

Choose a reason for hiding this comment

johndoknjas left a comment

Choose a reason for hiding this comment

johndoknjas commented Apr 20, 2023

knutole commented Apr 20, 2023

kieferro commented Apr 20, 2023

Refactor and add `num_nodes` and `verbose` options to `get_top_moves()` #16

Refactor and add `num_nodes` and `verbose` options to `get_top_moves()` #16

knutole commented Apr 7, 2023 •

edited

Loading

github-actions bot commented Apr 7, 2023 •

edited

Loading