search

maxpumperla / deep_learning_and_the_game_of_go

Code and other material for the book "Deep Learning and the Game of Go"
https://www.manning.com/books/deep-learning-and-the-game-of-go
989 stars 390 forks source link

goboard_slow.py method does_move_violate_ko() Incorrect #34

Open roryheim opened 5 years ago

roryheim commented 5 years ago

I believe the goboard_slow.py from chapter 3 has an error in checking for ko violation. The original code ends up only comparing player and the object id; the object id will always be different, so even when the new board is returned to the state that will violate ko, it won't be detected. The Zobrist code works fine because the hashes are directly compared. So this is minor because most people will want to use Zobrist anyway.

I added a test to the provided test code to prove this bug exists. Try the test against the old code, it should fail.

Arguably I should write a dunder eq for this fix, but I went with this approach first.

macfergus commented 5 years ago

Good catch, thank you for this fix! Will review this tomorrow

roryheim commented 5 years ago

Hi

I didn't mention it in the PR but I have been writing auts against ch3. The only issue is that I couldn't get your code to run correctly so my project structure is different. I do collect code coverage which is nice to ensure everything gets tested. The only other thing I have found is places where code is unreachable.

If you are interested in my auts I can invite you to my bitbucket repo.

Thanks Rory

On Mon, May 6, 2019, 9:27 PM Kevin Ferguson notifications@github.com wrote:

Good catch, thank you for this fix! Will review this tomorrow

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/maxpumperla/deep_learning_and_the_game_of_go/pull/34#issuecomment-489903206, or mute the thread https://github.com/notifications/unsubscribe-auth/AACA4VLB77SD54XQDB7NOELPUEAMDANCNFSM4HLEYJRQ .