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

Implement engine parameters UCI_Variant and Threads #6

Merged
merged 6 commits into from
Jan 3, 2018

Conversation

ddugovic
Copy link
Contributor

No description provided.

@rpdelaney
Copy link
Collaborator

Nice, thank you! I should be able to look into this more closely sometime next week.

A thought: Right now, chess-annotator determines if a move needs an annotation via calculations in needs_annotation() and winning_chances(). I built that based on my understanding of how lichess does it.

But I've noticed that stockfish's evaluations of crazyhouse games tend to swing a lot more than they do for chess. I don't know if that's because crazyhouse positions are inherently "sharper" than they are in chess, or if there's some other property of stockfish-zh that I don't understand that makes it that way.

Anyway, I'm anticipating that unless something changes, chess-annotator will be much more verbose about annotating crazyhouse games. (I'm not very skilled at other variants, so I know much less about them, but I imagine something similar might obtain in antichess.)

I'd be interested in hearing your thoughts on that. Do you think needs_annotation() should use a different value from 7.5 when working with variant games? I was also considering adding a command-line switch to enable the user to set the threshold directly, or choose from a few pre-set verbosity profiles.

@ddugovic
Copy link
Contributor Author

I'm unsure why Stockfish's evaluations of crazyhouse games fluctuate so much, but considering it just won the Computer Crazyhouse Championship it must be doing something right!

I do like your idea of parameterizing (command line switching) "winning chances delta" thresholds for each category of mistake (blunder, mistake, inaccuracy, ...?); maybe even allowing variant-specific thresholds?

@rpdelaney
Copy link
Collaborator

Congrats!

It seems we're on the same page. I'll look into adding options like that after this PR is merged.

On that topic:

  1. Maybe we should skip the opening classification if it's a variant game.
  2. I just tested it on this game: https://lichess.org/bIqjcbbb but it's throwing lots of exceptions about illegal uci moves. Were you able to get it to execute successfully on a variant game?

@rpdelaney
Copy link
Collaborator

Ah, I wasn't using the variant stockfish. Rookie mistake. Now it gets further, but it still died:

An unhandled exception occurred: <class 'KeyError'>
Traceback (most recent call last):
  File "/usr/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/ryan/src/games/annotator/annotator/__main__.py", line 703, in <module>
    main()
  File "/home/ryan/src/games/annotator/annotator/__main__.py", line 693, in main
    raise e
  File "/home/ryan/src/games/annotator/annotator/__main__.py", line 687, in main
    analyzed_game = analyze_game(game, args.gametime, args.engine, args.threads)
  File "/home/ryan/src/games/annotator/annotator/__main__.py", line 632, in analyze_game
    add_annotation(node, judgment)
  File "/home/ryan/src/games/annotator/annotator/__main__.py", line 286, in add_annotation
    variation = truncate_pv(node.board(), judgment["pv"])
  File "/home/ryan/src/games/annotator/annotator/__main__.py", line 267, in truncate_pv
    board.push(move)
  File "/home/ryan/.local/lib/python3.6/site-packages/chess/variant.py", line 651, in push
    self.pockets[self.turn].remove(move.drop)
  File "/home/ryan/.local/lib/python3.6/site-packages/chess/variant.py", line 604, in remove
    self.pieces[pt] -= 1
KeyError: 1

I'm not sure if this is a bug with chess-annotator or python-chess. I'll try to dig into it with a debugger when I get a chance.

@ddugovic
Copy link
Contributor Author

ddugovic commented Jan 1, 2018

I didn't attempt to perform opening classification for variants, although I believe a wealth of opening theory exists for crazyhouse, atomic, and antichess. That seems like a nice-to-have feature (although I'm curious why the KeyError occurs).

And thanks! I'm looking forward to letting Lichess users know about this game annotator; maybe in the future I'll find a way to integrate it with Lichess studies

@rpdelaney
Copy link
Collaborator

rpdelaney commented Jan 2, 2018

I reached out to Niklas to see if he has any feedback on that KeyError exception.

Some more test results:

  1. 3-check seems to work: http://dpaste.com/3V308TM
  2. chess960 throws the error not in UCI_Chess960 mode but position has non-standard castling rights and eventually crashes with an EngineTerminatedException. I think UCI_Chess960 needs to be set separately, which means we'll need additional variant detection logic when setting the UCI options: https://python-chess.readthedocs.io/en/latest/variant.html#uci
  3. antichess seems to work: http://dpaste.com/0F39SHB Note that in python-chess there are supported variants for "suicide" and "giveaway" but there is no "antichess". I'm not sure which corresponds to which. It might be a good idea to test this with more antichess games.
  4. atomic seems to work: http://dpaste.com/3YRHBD2
  5. racing kings seems to work: http://dpaste.com/19ZNWH5
  6. horde seems to work: http://dpaste.com/0VD43P5

I think that's everything?

I don't have a pgn viewer that can handle these variants, which complicates testing since it's a bit of a pain to verify that the engine variations are appropriate to the variant type. Lichess can import pgn of variant games, but it strips the annotations and variations, diminishing its value.

@rpdelaney
Copy link
Collaborator

@ddugovic Do you know how I can push commits to this PR? Or would I have to be a maintainer of your fork?

@ddugovic
Copy link
Contributor Author

ddugovic commented Jan 3, 2018

@rpdelaney I think it is possible even without being a maintainer (as I have "Allow edits from [upstream] maintainers" selected).

I assume this is how it works:

git remote add ddugovic git@github.com:ddugovic/python-chess-annotator.git
git checkout ddugovic/uci_variant
...
git push ddugovic uci_variant

@rpdelaney
Copy link
Collaborator

For my future reference, since I mucked this up a few times, I had to do this:

git remote add ddugovic git@github.com:ddugovic/python-chess-annotator.git
git fetch --all
git checkout ddugovic/uci_variant
...
git push ddugovic HEAD:uci_variant

Chess 960 seems to work with the last commit. Also, the KeyError I observed above is the result of a bug in my code that is now fixed in the master branch. I'm going to do a little bit more testing but this should be ready to merge in soon.

Ryan Delaney added 3 commits January 3, 2018 11:12
The board().uci_variant object is always populated, because in non-
variant games the uci_variant is "chess". Therefore, we should check
that the uci_variant is not "chess" before passing UCI_Variant options
to the engine.
board().variant will be "chess" if the game is 960, so a separate
check is required for 960 games.
We don't have anything like ECO for variant games, so an ECO
classification of a variant game is very likely to be meaningless.
@rpdelaney rpdelaney merged commit a23f3c9 into rpdelaney-archive:master Jan 3, 2018
@rpdelaney
Copy link
Collaborator

Thanks again!

@ddugovic ddugovic deleted the uci_variant branch January 3, 2018 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants