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

[RFC] Make UCI option handling more type-safe and the code cleaner. #3674

Closed
wants to merge 1 commit into from

Conversation

Sopel97
Copy link
Member

@Sopel97 Sopel97 commented Aug 24, 2021

This patch cleans up the syntax and semantics of the code handling UCI options. The following changes have been done:

  • add a proper OptionsMap class that holds both ordered an unordered options
    • prevents accidental O(n^2) when printing the options. If my understanding is correct this could be an issue with a lot of tunable parameters.
  • the allowed combo values are parsed on option creation instead of each assignment
  • the accessors are explicitly typed to prevent prevent ambiguity, casting, and to know the type on the caller side
  • Options was moved to the UCI namespace
  • NNUE::verify no longer uses a hacky way of getting the default nnue file name
  • an allowEmpty field is added to each option, see "allowEmpty" property for UCI options. Disallow empty values for net file. #3668
  • chain syntax for initializing optional fields in the option (onChange, allowEmpty)
  • named constructors for creating options
    • include the option type in the name, no need to do the mental work of resolving the overloads
  • get rid of a stringly-typed option type in favor of a closed set of types (enum)
  • correctly use stod instead of stof for reading double options
  • round instead of truncate when reading double option values as int
  • don't abuse operator<< for adding options to the OptionsMap

I want to get some feedback regarding this, which changes are good and which are bad in your eyes, and whether there's a chance of getting this merged. If this is to be merged we will need to test for non-regression on fishtest and also verify whether the tuning works correctly.

@MichaelB7
Copy link
Contributor

MichaelB7 commented Sep 4, 2021

There are a lot changes. I suggest testing for non regression first before debating the merits. Also, rather than making one big patch , perhaps numerous small patches that address each issue would facilitate the review by the maintainers. I would then prioritize the sequencing of the patches by the changes you believe are most important first. As it stands now, the patch has over 400 lines of code changes for what appears to be little value other than improving syntax and semantics. I'm not dimimshing the value of syntax and semantics, but all of the code changes are codes changes which were reviewed previously and were deemed appropriately syntaxed and semanticized.

@Sopel97 Sopel97 closed this Sep 23, 2021
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