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

Adjust default MLH settings and unhide options for next release. #1328

Merged
merged 32 commits into from
Jun 28, 2020

Conversation

AlexisOlson
Copy link
Contributor

Having hidden options that are set to not-recommended defaults is a major usability issue.

The defaults here still have MLH disabled but only require changing two parameters to match the settings recommended by the current !mlh bot command on Discord:

MovesLeftMaxEffect 0.15
MovesLeftThreshold 0.3
MovesLeftSlope 0.015
MovesLeftQuadraticFactor 1.0
MovesLeftConstantFactor 0.0
MovesLeftScaledFactor 0.0

AlexisOlson and others added 30 commits April 18, 2020 13:50
* Appended files.

* Compiles.

* Compiles again.

* Make smart pruning use smoothed nps.

* Seems to be fully implemented.

* Mistype.

* One more bug.

* Found discrepancy with documentaiton.

* Bugfixes.

* Don't smooth nps during the first move.

* Too large default for timeuse decay.

* Bugfix.

* Fix build.

* Relax defaults a bit. Add fixed to logging.

* Remove "smooth" to "smooth-experimental" for now.
* Add M effect logic to output section

* Fix missing prefixes and semicolons

* Some fixes.

* Slight format improvement?

Co-authored-by: Tilps <Tilps@users.noreply.github.com>
* Added TempDecayStartMove for starting temp decay only after a given number of moves. This allows keeping initial game up for a few moves and still use decay.

* Doesn't allow temperature to fall below endgame temp during temp decay. Still allows initial temp to be below endgame temp.

* Doesn't allow temperature to fall below endgame temp during temp decay. Still allows initial temp to be below endgame temp.

* Hide temp options

* renamed TempDecayStartMove to TempDecayDelayMoves

Co-authored-by: Alexis Olson <AlexisOlson@gmail.com>
* Changelog for 0.25.0-rc2.

* Add one more PR to the changelog.
* custom winograd convolution for cuda backends

* custom winograd fixes

- fix a bug to make it work for non-SE networks
- enable by default only with fp32.

* address review comments

* remove random line in comment

* remove unused constants

- W,H are hardcoded to 8 - because there are assumptions in the code based on that. No point in defining constants.
* cuda winograd fixes
- don't typecast directly to half datatype in CPU side code as older CUDA runtime doesn't support that.
 - don't use gemmEx version on GPUs older than Maxwell generation (not supported).
 - modify the check to enable custom_winograd setting. It should be faster in most cases - except  presently on RTX GPUs when using fp16.
Default to white to move, no castling, no en passant, 0 rule50ply, 1 total move. Also convert other string to std::string and removing using.
* memory optimization for cudnn custom_winograd

- don't save untransformed weights
- print warning message when low memory is detected.

* address review comments

* fix warning message

* fix total weight size calculation

2 layers per residual block!
Use printing lambdas for parts of the verbose output to share between the newly outputted node and its children.
…ero#1284)

* Allow smart pruning to terminate search if win is known.

* Minor tweak, better safe than sorry.
* Time management refactoring (LeelaChessZero#1195)

* Appended files.

* Compiles.

* Compiles again.

* Make smart pruning use smoothed nps.

* Seems to be fully implemented.

* Mistype.

* One more bug.

* Found discrepancy with documentaiton.

* Bugfixes.

* Don't smooth nps during the first move.

* Too large default for timeuse decay.

* Bugfix.

* Fix build.

* Relax defaults a bit. Add fixed to logging.

* Remove "smooth" to "smooth-experimental" for now.

* MLH verbose stats - Issue 1200  (LeelaChessZero#1230)

* Add M effect logic to output section

* Fix missing prefixes and semicolons

* Some fixes.

* Slight format improvement?

Co-authored-by: Tilps <Tilps@users.noreply.github.com>

* Start TempDecay only after a given number of moves (LeelaChessZero#1212)

* Added TempDecayStartMove for starting temp decay only after a given number of moves. This allows keeping initial game up for a few moves and still use decay.

* Doesn't allow temperature to fall below endgame temp during temp decay. Still allows initial temp to be below endgame temp.

* Doesn't allow temperature to fall below endgame temp during temp decay. Still allows initial temp to be below endgame temp.

* Hide temp options

* renamed TempDecayStartMove to TempDecayDelayMoves

Co-authored-by: Alexis Olson <AlexisOlson@gmail.com>

* Changelog for 0.25.0-rc2. (LeelaChessZero#1233)

* Changelog for 0.25.0-rc2.

* Add one more PR to the changelog.

* Cuda winograd (LeelaChessZero#1228)

* custom winograd convolution for cuda backends

* custom winograd fixes

- fix a bug to make it work for non-SE networks
- enable by default only with fp32.

* address review comments

* remove random line in comment

* remove unused constants

- W,H are hardcoded to 8 - because there are assumptions in the code based on that. No point in defining constants.

* cuda winograd fixes (LeelaChessZero#1238)

* cuda winograd fixes
- don't typecast directly to half datatype in CPU side code as older CUDA runtime doesn't support that.
 - don't use gemmEx version on GPUs older than Maxwell generation (not supported).
 - modify the check to enable custom_winograd setting. It should be faster in most cases - except  presently on RTX GPUs when using fp16.

* Allow most parts of fen to be optional. (LeelaChessZero#1234)

Default to white to move, no castling, no en passant, 0 rule50ply, 1 total move. Also convert other string to std::string and removing using.

* Fix UpdateNps to actually smooth the nps and correctly handle time_since_movestart_ms == 0 (LeelaChessZero#1243)

* Update changelog for 0.25.0 final release. (LeelaChessZero#1244)

* Always report at least 1 depth. (LeelaChessZero#1247)

* Fix un-intended regression for GTX GPUs (LeelaChessZero#1246)

* memory optimization for cudnn custom_winograd (LeelaChessZero#1250)

* memory optimization for cudnn custom_winograd

- don't save untransformed weights
- print warning message when low memory is detected.

* address review comments

* fix warning message

* fix total weight size calculation

2 layers per residual block!

* keep pdb files only for release builds  (LeelaChessZero#1256)

* doc update (LeelaChessZero#1267)

* Include verbose stats for the node. (LeelaChessZero#1268)

Use printing lambdas for parts of the verbose output to share between the newly outputted node and its children.

* add alphazero time manager (LeelaChessZero#1201)

* Updated FLAGS.md with logfile flag (LeelaChessZero#1275)

* Fixed a typo in CONTRIBUTING.md (LeelaChessZero#1274)

* Update Readme about using git (LeelaChessZero#1265)

* Make `wl_` double. (LeelaChessZero#1280)

* Move move filter population to a constructor. (LeelaChessZero#1281)

* Filter out illegal searchmoves to avoid crashing. (LeelaChessZero#1282)

* Clear policy for terminal loss. (LeelaChessZero#1285)

* Allow smart pruning to terminate search if win is known. (LeelaChessZero#1284)

* Allow smart pruning to terminate search if win is known.

* Minor tweak, better safe than sorry.

* Fix bug where pv might not update for best move change. (LeelaChessZero#1286)

* Fix bug where pv might not update.

* Fix...

Co-authored-by: Alexander Lyashuk <crem@google.com>
Co-authored-by: Tilps <Tilps@users.noreply.github.com>
Co-authored-by: Naphthalin <40385638+Naphthalin@users.noreply.github.com>
Co-authored-by: Ankan Banerjee <ankan.ban@gmail.com>
Co-authored-by: Ed Lee <edilee@mozilla.com>
Co-authored-by: borg323 <39573933+borg323@users.noreply.github.com>
Co-authored-by: Hace <mellekoning@gmail.com>
Co-authored-by: Kip Hamiltons <48076495+KipHamiltons@users.noreply.github.com>
Co-authored-by: nguyenpham <axchess@yahoo.com>
@Naphthalin
Copy link
Contributor

The above values were tuned for 0.07 slope for T70, which now uses a sharper slope so whenever this is relevant, policies should already be sharper. I therefore would prefer slope=0.01 as the default value and not higher. Default threshold should be 0.99 or so so it would be off by default (until terminal nodes), and to activate it we only need to reduce the threshold. Also, I am strongly against unhiding the 3 factors, once we have reasonable default values. These were only intended to adjust the Q curve without having to change the binary.

@AlexisOlson
Copy link
Contributor Author

A different slope is fine. We can adjust to whatever is considered best when it comes time to merge.

I left the threshold at 1.0 because since MLH is disabled by default, the do_moves_left_adjustment boolean should be false to prevent unnecessary calculation of M_effect. It might make sense to add an m_cap > 0.0 condition in the definition of do_moves_left_adjustment. Then we could set a default threshold of less than 1.0 without triggering do_moves_left_adjustment when m_cap = 0 (and you'd only need to adjust one parameter to use MLH with recommended settings).

Long term, hiding the 3 factors is fine but they should be unhidden as long as there isn't a clear consensus on their settings. It's not clear to me whether or not we have that consensus yet. If there is consensus, I'm fine with keeping them hidden.

@mooskagh
Copy link
Member

mooskagh commented Jun 7, 2020

I'm not sure about unhiding part, especially given that there will be an easier way to unhide all hidden params ("pro version").
Actually, I'm more for hiding more parameters that most of people don't touch. There were multiple complaints about that including from top chess players.

@AlexisOlson
Copy link
Contributor Author

Hidden is fine as long as they don't need tweaking for best results.

However, it seems like best results for the factor settings aren't completely settled yet:
https://discord.com/channels/425419482568196106/530486338236055583/719100480885751808

@Tilps Tilps added the release blocker Bugs which block releases label Jun 24, 2020
src/mcts/params.cc Outdated Show resolved Hide resolved
src/mcts/params.cc Outdated Show resolved Hide resolved
@Naphthalin
Copy link
Contributor

It seems like you pushed an old branch again, as the MLH options are unhidden again?

@AlexisOlson
Copy link
Contributor Author

@Naphthalin No, I never re-hid them as it hasn't been clear which factor values should be the default. Is there a consensus that quadratic = 1 should be default with other options not directly accessible?

@Tilps
Copy link
Contributor

Tilps commented Jun 28, 2020

I think we can argue about hide vs unhide while the RC process is going on - most important is the default values are improved.

@Tilps Tilps dismissed Naphthalin’s stale review June 28, 2020 03:39

Requested changes were applied and I'm approving as forward progress.

@Tilps Tilps merged commit 2511d11 into LeelaChessZero:master Jun 28, 2020
@AlexisOlson AlexisOlson deleted the mlh-defaults branch June 28, 2020 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release blocker Bugs which block releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants