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

Cleanup and simplify NNUE code. #3441

Closed
wants to merge 1 commit into from

Conversation

Sopel97
Copy link
Member

@Sopel97 Sopel97 commented Apr 24, 2021

A lot of optimizations happend since the NNUE was introduced and since then some parts of the code were left unused. This got to the point where asserts were have to be made just to let people know that modifying something will not have any effects or may even break everything due to the assumptions being made. Removing these parts removes those inexisting "false dependencies". Additionally:

  • append_changed_indices now takes the king pos and stateinfo explicitly, no more misleading pos parameter (ideally the king square would be a part of StateInfo, or some NNUE specific part of StateInfo (but more general than dp), but whatever)
  • IndexList is removed in favor of a generic ValueList. Feature transformer just instantiates the type it needs.
  • The update cost and refresh requirement is deferred to the feature set once again, but now doesn't go through the whole FeatureSet machinery and just calls HalfKP directly.
  • accumulator no longer has a singular dimension
  • The PS constants and the PieceSquareIndex array are made local to the HalfKP feature set because they are specific to it and DO differ for other feature sets.
  • a few names are changed to more descriptive

@vondele
Copy link
Member

vondele commented Apr 24, 2021

can you run a non-regression test for that on fishtest.

@Sopel97
Copy link
Member Author

Sopel97 commented Apr 24, 2021

@vondele
Copy link
Member

vondele commented Apr 25, 2021

which passed

 LLR: 2.95 (-2.94,2.94) <-2.50,0.50>
Total: 180008 W: 16186 L: 16258 D: 147564
Ptnml(0-2): 587, 12593, 63725, 12503, 596 

So, that looks like a good cleanup to me.

@vondele vondele added the to be merged Will be merged shortly label Apr 25, 2021
@vondele vondele closed this in b748b46 Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants