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

Fix bug with "excludedMove" for probcut #1754

Closed
wants to merge 6 commits into from

Conversation

MJZ1977
Copy link
Contributor

@MJZ1977 MJZ1977 commented Aug 28, 2018

Bugfix :
"excludedMove" must be skipped in probcut loop too. If it is not skipped, the probcut can exit quickly with wrong return value corresponding to the excluded move.

See (https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU)

STC :
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

LTC :
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 12387 W: 2064 L: 1930 D: 8393
http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21

To go further : it can be perhaps useful to tune the singular extension search parameters.

Bench: 4694445

@pb00068
Copy link
Contributor

pb00068 commented Aug 28, 2018

@MJZ1977 Congratulations for the patch. The continuous integration check failed because you missed to terminate your commit message with the bench-number.
Your commit message should be terminate with:
bench: 4694445

@jerrydonaldwatson
Copy link

Congratulations, this is a good catch!

@MJZ1977
Copy link
Contributor Author

MJZ1977 commented Aug 28, 2018

I added bench number. Is it OK like this?

@pb00068
Copy link
Contributor

pb00068 commented Aug 28, 2018

@MJZ1977 Can't see it.
I suggest you to make and push a new commit (containing the bench commit-message) on this branch,
so the continuous-integration test will be triggered again automatically.

@MJZ1977
Copy link
Contributor Author

MJZ1977 commented Aug 28, 2018

Must I make a new branch from the beginning ?

… not skipped, the probcut can exit quickly with wrong return value corresponding to the excluded move. See (https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU)  STC : http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a ELO +2.19  LTC : http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21 ELO +3.4  To go further : it can be perhaps useful to tune the singular extension search parameters.  Bench: 4694445
@pb00068
Copy link
Contributor

pb00068 commented Aug 28, 2018

@MJZ1977 No, not necessary, but app-veyor is still failing.
Just redo as you did, but put the bench into commit-message as separate (last) line, then it should be ok ;-)

@MJZ1977
Copy link
Contributor Author

MJZ1977 commented Aug 28, 2018

Can you please make it, I am not familiar with all these tools.

EDIT: OK I understand now, what you call "message" is called "description" in Github desktop... I changed only the title in 1 line before. :D

… not skipped, the probcut can exit quickly with wrong return value corresponding to the excluded move. See (https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU)  STC : http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a ELO +2.19  LTC : http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21 ELO +3.4  To go further : it can be perhaps useful to tune the singular extension search parameters.  Bench: 4694445
"excludedMove" must be skipped in probcut loop too. If it is not skipped, the probcut can exit quickly with wrong return value corresponding to the excluded move.

See (https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU)

STC : http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a
ELO +2.19

LTC : http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21
ELO +3.4

To go further : it can be perhaps useful to tune the singular extension search parameters.

Bench: 4694445
@locutus2
Copy link
Member

Congrats too! Nice that you found the bug.

The bug was introduced with this simplification 4d64742. Before this the node pruning methods were skipped if we had an excluded move.

Please add to your PR message not the elo value but the complete result like showed on the test page (simply copy paste). See as example the above commit

For your STC this could look like following:

STC : 
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

Also i would recommend @Rocky640's code rewrite.

@MJZ1977
Copy link
Contributor Author

MJZ1977 commented Aug 28, 2018

Considering @Rocky640 proposition, it is not a simple rewrite because the loop will stop after we met the excluded move.
Edit : changed after Rocky Edit

Bench 4694445
@DU-jdto
Copy link

DU-jdto commented Aug 28, 2018

Seems weird to write it this way instead of just if (move != excludedMove && pos.legal(move))

@MJZ1977
Copy link
Contributor Author

MJZ1977 commented Aug 28, 2018

@DU-jdto I agree with you.
I let the maintainers choose the right way to write it. The most important for me is to correct the bug and go ahead. How many time it will need in general to merge with master?

@snicolet snicolet changed the title Exclude "excludedMove" from probcut Fix bug with "excludedMove" for probcut Aug 29, 2018
@snicolet
Copy link
Member

Merged via 10bb2e6, thanks!

@snicolet snicolet closed this Aug 29, 2018
snicolet pushed a commit that referenced this pull request Aug 29, 2018
Bugfix: "excludedMove" has to be skipped in the probcut loop too.
If it is not skipped, the probcut can exit quickly with a wrong return
value corresponding to the excluded move. See the following forum
thread for a discussion:
https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU

STC :
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

LTC :
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 12387 W: 2064 L: 1930 D: 8393
http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21

To go further : it can be perhaps useful to tune the singular extension
search parameters.

Closes #1754

Bench: 4308541
@snicolet
Copy link
Member

How many time it will need in general to merge with master?

I suppose you mean "How much time does it need in general to merge with master?"

It depends if the maintainer is in vacation or not, but as a data point I can say that for
each pull request it takes me in general 45 minutes to import the patch, shrink the
multiple parts together, rewrite the code to make the style more coherent with Stockfish
code, correct the English mistakes of the message (and add my own), triple check the
bench, run with the DEBUG flag, push to official master, close the pull request, etc.

mstembera pushed a commit to mstembera/Stockfish that referenced this pull request Sep 3, 2018
Bugfix: "excludedMove" has to be skipped in the probcut loop too.
If it is not skipped, the probcut can exit quickly with a wrong return
value corresponding to the excluded move. See the following forum
thread for a discussion:
https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU

STC :
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

LTC :
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 12387 W: 2064 L: 1930 D: 8393
http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21

To go further : it can be perhaps useful to tune the singular extension
search parameters.

Closes official-stockfish#1754

Bench: 4308541
mstembera pushed a commit to mstembera/Stockfish that referenced this pull request Sep 4, 2018
Bugfix: "excludedMove" has to be skipped in the probcut loop too.
If it is not skipped, the probcut can exit quickly with a wrong return
value corresponding to the excluded move. See the following forum
thread for a discussion:
https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU

STC :
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

LTC :
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 12387 W: 2064 L: 1930 D: 8393
http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21

To go further : it can be perhaps useful to tune the singular extension
search parameters.

Closes official-stockfish#1754

Bench: 4308541
lantonov pushed a commit to lantonov/Stockfish that referenced this pull request Sep 30, 2018
Bugfix: "excludedMove" has to be skipped in the probcut loop too.
If it is not skipped, the probcut can exit quickly with a wrong return
value corresponding to the excluded move. See the following forum
thread for a discussion:
https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU

STC :
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

LTC :
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 12387 W: 2064 L: 1930 D: 8393
http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21

To go further : it can be perhaps useful to tune the singular extension
search parameters.

Closes official-stockfish#1754

Bench: 4308541
mstembera pushed a commit to mstembera/Stockfish that referenced this pull request Oct 18, 2018
Bugfix: "excludedMove" has to be skipped in the probcut loop too.
If it is not skipped, the probcut can exit quickly with a wrong return
value corresponding to the excluded move. See the following forum
thread for a discussion:
https://groups.google.com/forum/?fromgroups=#!topic/fishcooking/GGithf_VwSU

STC :
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17130 W: 3747 L: 3617 D: 9766
http://tests.stockfishchess.org/tests/view/5b8460c40ebc5902bdbb999a

LTC :
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 12387 W: 2064 L: 1930 D: 8393
http://tests.stockfishchess.org/tests/view/5b8466f90ebc5902bdbb9a21

To go further : it can be perhaps useful to tune the singular extension
search parameters.

Closes official-stockfish#1754

Bench: 4308541
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.

None yet