Add new game: Nim

[acforvs]

Hey OpenSpiel team,

I would love to address the recent Call for New Games #843 and suggest this implementation of the game Nim :)

Looking forward to any comments

[lanctot]

Thanks!

The test failure seems unrelated to your PR -- we'll fix it separately and then ask you to pull from master.

Also, we've been a bit busy and are a bit behind on importing PRs so there might be some delay.

[acforvs]

Okay, thanks!

[lanctot]

Hi @acforvs,

Ok we fixed these errors in #869, which is now in master. Can you pull changes from master and commit the merge to trigger the tests again with these changes?

[lanctot]

I don't see a merge commit? I think you will have to pull the changes that were pushed to master just a few hours ago.

[lanctot]

Specifically, these: 6a49b93

[acforvs]

@lanctot should be fixed, I accidentally rebased from my fork's master which wasn't up to date

[acforvs]

@lanctot wheels failed again.. could you take a look please once you have a moment?

[acforvs]

I think the problem might be in https://github.com/deepmind/open_spiel/blob/master/open_spiel/games/euchre.cc#L476, should I create a separate PR in order to fix it?

upd: looks like the reason is in recent commits 15145eb and 910cf6d after all

[lanctot]

Yeah that's on our side -- the wheel tests are more restrictive and we don't have an easy way to test this internally. It's quite rare to encounter these, you've been really unlucky. Should be fixable -- I'll give it a shot.

[lanctot]

Unless I made a typo or incorrect assumption, I believe this will fix it: #870

(You can apply that locally in your branch and commit it, because the person I'd like to ask to review it is on holiday and I will soon be too. It's fine if it's duplicated from master because our import tool can detect that and won't cause any conflicts.)

[acforvs]

Great, thank you! I will wait for the tests there to pass and apply that code locally
Enjoy your holiday by the way :)

[lanctot]

Yeah hit a few bumps. I've tried one last thing for tonight that I think will work, but it might not be what the Euchre author prefers, so I'll wait until we can pull it in properly to ask you to rerun. Given our schedules, might only be in a few weeks.

[lanctot]

The fixes in #870 will require some more thought. I will tag the original author and will update you when we have it fixed.

[lanctot]

Alright, update: #873 will fix the last few test failures, but it will not be merged for a while (and once it's done will require you to run ./install.sh again after pulling from master). So, I think with the other tests passing, no need to worry about this, we can import it as is. However, it might take some time for reviews due to scheduling.

[acforvs]

Great, thanks! Could I do the same as you suggested here #868 (comment) and apply those changes locally prior to the merge?

[lanctot]

Yes, that should work if you want to try it.

[lanctot]

Can you add the game and its description to docs/games.md?

[lanctot]

Hi @acforvs, just a heads-up: I have already imported this PR so that last commit won't make it (and any subsequent changes to your branch won't either.)

I did a few style fixes on our end during the import.

Now I'm just waiting on an internal review and once it's in, it should be included with the next github sync.

[acforvs]

@lanctot oh okay, thanks! I thought that since there were no "imported" tag I could still make some adjustments :)

[lanctot]

@lanctot oh okay, thanks! I thought that since there were no "imported" tag I could still make some adjustments :)

Yeah.. it's a good point. I normally only set the "imported" label once the internal review is done and it's submitted into our system (because I didn't want authors to be confused if it didn't show up on the sync)... and it's never made a difference.

But I think I'll do that from now on and change the label text for better communication. Sorry about that!