Consider changing the return type of `get_wdl_stats` to Optional[Tuple[int,int,int]]

[johndoknjas]

Might be something we want to do, but not entirely sure.

[kieferro]

I would think that would be nicer than working with lists. The only problem I'd see is that someone would read out the WDL-stats and then modify them somehow, which wouldn't work with tuples. But I doubt that's a common use case.

[evanjhoward11]

Hi @johndoknjas and @kieferro, I am a first-time contributor. Can I take a crack at this?

Is it a matter of converting the return expression from a list to a tuple? For example: return tuple(existing expression)

[johndoknjas]

Hi @evanjhoward11 for sure, any contributions are appreciated. What I'd recommend is the following:

• Based on what @kieferro mentioned in his comment above last year, in order to ensure backwards compatibility we should probably by default still return a list. However, adding an optional bool parameter (e.g., get_as_tuple or something) that's by default set to False could work. Then existing users of the function will still get a list as they expected, while new users will see the parameter in their IDEs when calling the function, and can decide for themselves.
• Updating the function's docstring to describe the new bool param, and also updating the sentence describing what's returned.
• The return type of the function will have to be updated from Optional[List[int]] to something like Union[Optional[List[int]], Optional[Tuple[int,int,int]]]. Or perhaps Union[List[int], Tuple[int,int,int], None] is cleaner.
• For what you mentioned about tuple(existing expression), I believe yes that should be all that's needed to return a tuple version.
• get_wdl_stats is tested in tests/stockfish/test_models.py. So any tests could be updated to test the tuple version as well.

Besides that, some general things for contributing to this repo:

• Using pip to install black and mypy on your machine is recommended. You can then run them on any .py files you change.
• Instructions for writing tests and running them can be found in the readme under "Testing".

[evanjhoward11]

@johndoknjas, I've been working on your suggestions above, but have some questions.

1. How do I run the models.py file (or the project as a whole) so I can play around with it and test my changes?
2. When a tuple is returned, mypy shows it's Tuple[int, ...] instead of the expected Tuple[int, int, int]. I did a little research, but couldn't figure out why this is or how to change it. Might be an inherent characteristic of tuples?
3. I added a 4th test in test_models.py that only tests if the return type is a tuple. Do you think it is necessary to instead repeat the original 3 tests but with tuple as the return type (for a total of 6 tests: 3 with list returned, 3 with tuple returned)?

Thanks in advance!

[johndoknjas]

Hi @evanjhoward11, sorry for the late reply. For 2) and 3), I've addressed them in my review to your PR. For 1), what I've done on my local environment is to make a main.py in the stockfish folder. In this file, you can put at the top from models import Stockfish. This will allow you to test the changes you've made to models.py in the same directory.

Inside the stockfish folder on my local, I also added a .gitignore that ignores main.py, as well as this .gitignore itself. You could also add stockfish exes in this folder, and ignore them with this new .gitignore on your local.

[evanjhoward11]

I made the changes based on your comments and left a comment on the pull request. Thanks for explaining how to test the changes that I make! The code on PR #70 was really helpful, too.

On a separate note, while testing get_wdl_stats I noticed something a little odd. When I call the function multiple times in a row, it returns different numbers/results each time. Initially, I tried to fix this with time.sleep(0.1), but it had no effect. What ended up working was setting the fen position each time get_wdl_stats was called instead of once at the beginning. Whether the function returned a list or a tuple did not affect the output.

Not sure if this is expected or not. If it's a big enough problem, I can create a new issue.

[johndoknjas]

Hi @evanjhoward11 sorry for the delayed reply, and thanks for making the changes! For the function returning different outputs, that's actually expected so no worries there. I ran stockfish directly through the terminal, and entering go depth 18 twice gave two different wdl results.

It's likely because stockfish stores information from its previous search in a hash table, and uses that info for any subsequent searches that can benefit from it. When you called set_fen_position again, by default the send_ucinewgame_token param is True, telling our wrapper to send the ucinewgame command to Stockfish (resetting the hash table). This is why doing that gave consistent wdl results.

I reviewed why that test was failing for your PR, and it seems like the same issue as here. I'll just update the test to reset the hash table before calling get_wdl_stats again, and it should be good.