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

Disable tree reuse in training. #536

Merged
merged 11 commits into from
May 8, 2018
Merged

Conversation

Tilps
Copy link
Contributor

@Tilps Tilps commented May 5, 2018

This is an alternative to #528, since I didn't find any significant performance difference and this is even more clearly a win for noise effectiveness.

Tilps added 5 commits April 29, 2018 09:57
It was spinning on resign, if the 450th ply was a win/loss/draw it
wasn't recognized, and if the 450th ply was a win/loss/draw it was
trying to play a move from that position.
Doesn't seem to have a significant performance win, and has detrimental
effects on the effectiveness of noise.
@Tilps
Copy link
Contributor Author

Tilps commented May 5, 2018

Note that this PR is diffbased against PR #526 - since its easier to make this change after that is fixed.

@killerducky
Copy link
Collaborator

I'm in favor of this, especially since it's practically free performance-wise. I'd like to see some opinions from @glinscott @Error323.

@Tilps
Copy link
Contributor Author

Tilps commented May 5, 2018

Some numbers to aid consideration.
First move with current net (id245). 444 visits are reused after the first move if temperature chooses e4 (which is more likely than not).
This is not abnormally high either, 10 or so moves in to a random game I found a case with 780 visits reused. That's only 20 visits with noise applied to try and make a discovery.

src/UCI.cpp Outdated
for (int game_ply = 0; game_ply < 450; ++game_ply) {
auto search = std::make_unique<UCTSearch>(bh.shallow_clone());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a comment saying we are doing this on purpose to avoid tree reuse? Otherwise it might look like a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Tilps
Copy link
Contributor Author

Tilps commented May 5, 2018

I've started gathering some data. Moves is actually ply. baseline and baseline 2 are independent searches with full tree reuse policy visit delta relative to a main full tree reuse search that is in charge of choosing the moves under standard training conditions. I put in 2 baselines to get a feel for what kind of error bar there is in the data.
dual_tree is same but with the alternating trees approach from #528.
nohistory (poor name) is same again but with no tree reuse.
Delta moves: 2614 baseline: 426416, baseline2: 429458, dual_tree: 462418, nohistory: 469090
Delta moves: 2615 baseline: 426694, baseline2: 429562, dual_tree: 462590, nohistory: 469260
Delta moves: 2616 baseline: 426920, baseline2: 429730, dual_tree: 462746, nohistory: 469456
Delta moves: 2617 baseline: 427132, baseline2: 429898, dual_tree: 462904, nohistory: 469572
Delta moves: 2618 baseline: 427132, baseline2: 429898, dual_tree: 462904, nohistory: 469572
Delta moves: 2619 baseline: 427132, baseline2: 429898, dual_tree: 463098, nohistory: 469738
Delta moves: 2620 baseline: 427132, baseline2: 429898, dual_tree: 463098, nohistory: 469738
Delta moves: 2621 baseline: 427134, baseline2: 429898, dual_tree: 463336, nohistory: 469944

So disabling tree reuse is ~10% more noise effect overall. dual_tree is similar. I had another run about this deep which I accidentally closed - disabling tree reuse was a bit higher in that one, closer to 15% and dual tree was a bit closer to the middle. I'll leave it running overnight to see if it moves a bit more.

10-15% may not sound large - but I'll call out the specific lines I've selected above. They are taken from late game, and obviously there are some forced moves. Noise has 0 effect on a forced move, but then in the baseline it also has 0 effect on the next move, where dual tree and no tree reuse both get nice noise effects. So it may not be lots more noise overall, it is a huge increase in noise at specific moves which I think are the moves at most risk of policy overfit.

@Tilps
Copy link
Contributor Author

Tilps commented May 5, 2018

Overnight numbers: Not really much change. ~10% more for dual tree, and ~12% for no tree reuse.

Delta moves: 36909 baseline: 5351446, baseline2: 5328474, dual_tree: 5854904, nohistory: 6069182
Delta moves: 36910 baseline: 5351574, baseline2: 5328608, dual_tree: 5855048, nohistory: 6069312
Delta moves: 36911 baseline: 5351666, baseline2: 5328712, dual_tree: 5855178, nohistory: 6069442

@Tilps
Copy link
Contributor Author

Tilps commented May 5, 2018

I realized there was an issue with my methodology. Rather than comparing against the move chooser, I added a new search tree with no noise to compare against - I think its gives a more realistic value magnitude then calculating the diff against something with noise in it. Also included the move_chooser to compare next to baseline to see how much effect being the move chooser has on the result.

Early numbers:
Delta moves: 1300 move_chooser: 164278, baseline: 161462, baseline2: 162648, dual_tree: 179260, nohistory: 190058
Delta moves: 1301 move_chooser: 164320, baseline: 161498, baseline2: 162688, dual_tree: 179298, nohistory: 190100

Results are pretty similar, but the relative magnitude of the new scenarios are a bit higher than before.

@Tilps
Copy link
Contributor Author

Tilps commented May 6, 2018

One last result set. (I'm going to switch to testing something else now.) ~16% for no tree reuse and ~12% for dual tree.
Delta moves: 7676 move_chooser: 1003344, baseline: 996848, baseline2: 1003178, dual_tree: 1119180, nohistory: 1163256
Delta moves: 7677 move_chooser: 1003550, baseline: 997210, baseline2: 1003348, dual_tree: 1119456, nohistory: 1163606
Delta moves: 7678 move_chooser: 1003748, baseline: 997360, baseline2: 1003550, dual_tree: 1119650, nohistory: 1163868
Delta moves: 7679 move_chooser: 1003926, baseline: 997628, baseline2: 1003844, dual_tree: 1119842, nohistory: 1164094
Delta moves: 7680 move_chooser: 1004128, baseline: 997882, baseline2: 1003994, dual_tree: 1120030, nohistory: 1164320
Delta moves: 7681 move_chooser: 1004394, baseline: 998102, baseline2: 1004168, dual_tree: 1120250, nohistory: 1164552
Delta moves: 7682 move_chooser: 1004538, baseline: 998288, baseline2: 1004330, dual_tree: 1120402, nohistory: 1164684
Delta moves: 7683 move_chooser: 1004656, baseline: 998448, baseline2: 1004452, dual_tree: 1120512, nohistory: 1164824

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.

2 participants