Resolve 8 issues #89
Open
Resolve 8 issues #89
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
…lue is not just truthy.
johndoknjas
changed the title
ucinewgame command by default…commits too). So add `a2a4`.
…rting_position`.
johndoknjas
changed the title
Open
…f the set position functions. Rather, make a new function that can do this if the user wants.
…able is only a couple GBs, this can take more than half a second.
…necessary calls to `self._is_ready()`.
johndoknjas
changed the title
…t` function (since it is only used there).
johndoknjas
changed the title
johndoknjas
changed the title
…by not running in debug mode.
…moves function was modified to set the fen position after each move (in order to check for the correctness of each move). The issue with this is it will not allow the engine to detect game-state stuff like threefolds, even if the user sends all the moves in the game at once.
johndoknjas
changed the title
johndoknjas
force-pushed
the
ucinewgame
branch
from
2d0b61e to
eaf6a0a
Compare
johndoknjas
force-pushed
the
ucinewgame
branch
from
80b7412 to
1283378
Compare
johndoknjas
force-pushed
the
ucinewgame
branch
from
1283378 to
693cbd0
Compare
johndoknjas
force-pushed
the
ucinewgame
branch
from
e815fbe to
4e28a42
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #75, closes #77, closes #82, closes #83, closes #87, closes #88, closes #92, and closes #93.
In regards to issue #87, my
25473eb2commit in this PR implemented it. However, I then noticed that thetest_get_best_move_remaining_time_not_first_movefunction would fail with the version of SF used by the github ci. So in commit693cbd0I included "g1f3" as an acceptable move. After looking into it a bit, I found that in this instance SF gave "g1f3" due to no longer callingis_move_correctfrom the make moves function (which has SF do a depth 1 search for each move it's about to make). In a test PR I did to look into it, compare how a05ec3a fails but dbf0131 doesn't. Similarly, 6c6e391 fails but 695ae89 doesn't (for the linux tests).Since doing a depth 1 search before making each move certainly isn't required or expected in order to use Stockfish properly, adding "g1f3" to the test case is sound.
On the topic of issue #77:
As explained by the author of Fairy Stockfish here: "In fact, in Stockfish ucinewgame does exactly the same as setoption name Clear Hash".
For users, clearing the hash wouldn't be good to do when making moves, as it'll slow down a subsequent search. Also, clearing the table takes time itself. It only makes sense when starting a new game, but even then I suspect Stockfish's algorithm would clear it as it sees fit.
Removing the optional param of
set_fen_positiontechnically breaks backwards compatibility, since it's possible a user could call the function (without sending a second argument) and assume the hash table will implicitly be reset. However, this seems like a very rare case, and it's hard to see what the benefits would be. Since we're working on a new major release too, it feels justified in doing this. An alternative though is changing the name ofset_fen_positionso that no existing users can continue to call it, although I don't have a good candidate at the moment.