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

Notation conversion, get_num_pieces function, and misc. #112

Open
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

johndoknjas
Copy link
Contributor

@johndoknjas johndoknjas commented Aug 2, 2022

Note - since this PR is quite big, please merge the other PRs into master first. I'll then resolve any conflicts for this PR.

Closes #110

This PR allows the user to convert "human-style" notation into the notation format that Stockfish uses.

Thanks to @FalcoGer for providing the code for this feature in issue 110. I've refined it a bit and added some tests, but most of the credit should go to them.

Besides this, the PR also:

  • Doesn't send the "ucinewgame" command to the Stockfish engine, when setting a new position. I noticed that doing this frequently can end up being a large bottleneck (as Stockfish has to reset its transposition table each time). Instead, I've made a separate function which the user can call to send this command, if they want to.
  • Updates the readme to explain the get_top_moves and make_moves_from_current_position functions, as well as the Stockfish.Piece enum a bit.
  • Adds a function that lets the user find the number of pieces. If desired, the user can specify to only count the number of pieces in a certain range of file(s) and/or row(s). They also have the option of only counting certain pieces.
  • Modifies the _is_fen_syntax_valid function. Some changes include fixing a bug with splitting up the fields (use fen.split() instead of regexMatch.groups()), and adding checks for more types of invalid FENs (such as the halfmove clock field being >= 2x greater than the fullmove counter).

@github-actions
Copy link

github-actions bot commented Aug 2, 2022

Coverage report

Note
No coverage data of the default branch was found for comparison. A possible reason for this is that the coverage action has not yet run after a push event and the data is therefore not yet initialized.

The coverage rate is 92.55%.
The branch rate is 88%.

93.08% of new lines are covered.

Diff Coverage details (click to unfold)

stockfish/models.py

93.08% of new lines are covered (92.55% of the complete file).
Missing lines: 93, 193, 206, 578, 1093, 1099, 1109, 1113, 1115, 1119, 1127, 1131, 1133, 1135, 1160, 1178, 1190, 1226, 1237, 1244, 1245, 1267

johndoknjas and others added 3 commits August 7, 2022 01:11
…d, give the user an option to send it themselves with a separate function, if they want to do this.
@FalcoGer
Copy link

FalcoGer commented Aug 9, 2022

There are a few things I noticed.
In models.py line 897 there is the condition check

if (
                    isCapture
                    and turnsInto != ""
                    and self.get_what_is_on_square(dst) is None
                ) or (
                    isCapture
                    and turnsInto == ""
                    and self.will_move_be_a_capture(move)
                    == Stockfish.Capture.NO_CAPTURE
                ):

This was put like that because there was the bug (Issue#99) in self.will_move_be_a_capture(move) when move was 5 characters long (due to being a pawn turning into something else). Since that is fixed this part may be simplified now.

Also line 738 is a return "Empty Move", did you mean to raise a value error instead?

Also "+" and "#" at the end of the move string indicates a check/checkmate. I glossed over that in my implementation and just ignored it because I was just messing around, you may want to test if that's actually true.

Also there is a print statement warning that a move not describing a capture is in fact a capture. You may or may not want to remove that as printing to stdout is unexpected for such a function. Consider not allowing to capture pieces without explicitly declaring it to be a capture or swallowing it silently instead?

stockfish/models.py Outdated Show resolved Hide resolved
stockfish/models.py Show resolved Hide resolved
stockfish/models.py Show resolved Hide resolved
stockfish/models.py Outdated Show resolved Hide resolved
tests/stockfish/test_models.py Outdated Show resolved Hide resolved
stockfish/models.py Show resolved Hide resolved
stockfish/models.py Show resolved Hide resolved
README.md Show resolved Hide resolved
stockfish/models.py Outdated Show resolved Hide resolved
@FalcoGer
Copy link

FalcoGer commented Aug 9, 2022

I hope I didn't make too much of a mess. I'm not used to github's review system.

README.md Outdated Show resolved Hide resolved
@johndoknjas johndoknjas marked this pull request as draft August 9, 2022 12:13
@johndoknjas johndoknjas marked this pull request as ready for review August 9, 2022 19:06
@johndoknjas
Copy link
Contributor Author

@2union I'm very sorry to hear that, this is sad news. Thank you for letting us know though, we will try to continue the project. He started something that's become quite popular and useful for many people.

@FalcoGer
Copy link

FalcoGer commented Mar 6, 2023

@2union I'm very sorry to hear that, this is sad news. Thank you for letting us know though, we will try to continue the project. He started something that's become quite popular and useful for many people.

Maybe it's time for a fork?

@kieferro
Copy link
Contributor

kieferro commented Mar 6, 2023

Maybe it's time for a fork?

Yes, it definitely is. @johndoknjas and I already set up a new repo and intend to continue the development there. We haven't announced that yet since we're still in the middle of planning and getting more permissions for the PyPi package, but we plan to make the fork the main development point and move all PRs (and issues) that are still open here over there (Preferably not all at once so there is no chaos).

johndoknjas and others added 14 commits March 7, 2023 22:53
* Update some parameters to be of type bool rather than string.

* Fix bug related to stockfish needing lowercase 'false' and 'true' options.

* Add some backwards compatability safety checks in models.py, for the new bool types of the three params. Also update the README and .gitignore.

* Add docs for simpler way to call get_parameters.

* Do a different method for handling the get_parameters change. Instead, deprecate it in favour of a new function.

* Update some wording in the readme.

* Small edits to docs for recent changes.

* Update test function

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

---------

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

* Add examples; move classes

* Use __future__ annotation to allow putting classes on bottom

* Add pdoc; Add Github Workflow for publishing API docs on Github Pages

* Test Github Pages on PR

* Fix pip install in workflow

* Try fixing Github workflow

* Cleanup; add link to Github Pages in README

* Improve example

* Improve example

* Minor syntax fixes

* Update .github/workflows/api-docs.yml

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

---------

Co-authored-by: kieferro <81773954+kieferro@users.noreply.github.com>
…yabuzhsky#30)

* Add `debug_view` as option

* Add `debug_view` to documentation
* Works to make a new command to not run slow tests. However, get a warning message that testing like this via setup.py will be deprecated at some point.

* Change back some values.

* Format with black.

* Add slow marker to some of the tests.

* Update README.md
…ted docs and added warnings for weaker settings. (zhelyabuzhsky#38)

* Add functions to get the static evaluation, and resume full strength. Also updated some documentation.

* Update static eval test.

* Account for earlier versions of stockfish prefixing the static evaluation line with 'Total Evaluation'.

* Update README.md

* Issue warnings in get_top_moves, get_evaluation, and get_wdl_stats, when Stockfish is set to play on a weaker setting.

* Remove unnecessary calls to set the fen position.
johndoknjas and others added 3 commits May 31, 2023 16:58
* Add ': Stockfish' type hints in test_models.py.

* Fix mypy errors.

* Add various type hints in models.py.

* Minor test_models.py updates.
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.

FR - Include chess notation entry
5 participants