Stockfish: RENAMING/REFORMATTING THREAD

[mcostalba]

Use this issue to post renaming suggestions, once in a while, if approved will be committed at once.

We will use a table to make it easier account pending renames.

Please add your renames as new rows in the table. Thanks.

[mcostalba]

Proposed pending renames:

| Current name | Proposed rename | Rationale |
| :--- | :--- | :--- |
| shift(Bitboard b) | shift_bb(Bitboard b) | Consistency: all functions returning a
Bitboard are _bb suffixed |

[snicolet]

Good idea to open this issue for renaming, thanks!

I was actually thinking of opening some other permanent thread like that to accumulate the SMALL REFORMATS for a period, and test the accumulated set at the end of each period (say one month) as a normal double [-3..1] test: this would avoid slowing down the framework for zillions of small reformating edits, while still having the assurance that the reformats are not introducing hidden speed regressions.

Re: _bb suffix. My opinion is that the _bb suffix is fine for functions which actually _change_ the type of their parameter, to indicate that the codomain of the function is Bitboard.

But I think we should keep shift(Bitboard b), as it is so clean and readable, and is actually generalizing a shift of bitboard that already exists in C (the << operator) that everybody calls "shift".

[snicolet]

@mcostalba Can everybody edit the table? We don't have the credentials to edit comments, it seems...

[mcostalba]

@snicolet hmm indeed people is supposed to copy the table into a new one adding their row....

[mcostalba]

....maybe opening a wiki page, referenced by here could be better....

[jerrydonaldwatson]

Some more suggestions (can't edit table above, so posting here instead):

Current name | Proposed rename | Rationale
-- | -- | --
kingAdjacentZoneAttacksCount | kingAdjacentAttacksCount | The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop | LongDiagonalBishop | Slightly more accurate definition in my opinion.
TrappedBishopA1H1 | TrappedCornerBishop | Slightly more descriptive name in my opinion.
unstablePvFactor | unstableBestMoveFactor | Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode | RootNode | Consistency: we already have PvNode.

edited in view of comments below - thanks!

[mcostalba]

@jerrydonaldwatson thanks for your suugestions that I find good. Just to clarify regaridng capitalization convention:

• If it is a variable the name first char is lowercased
• If it is a template parameter or a constant the first char is uppercased

[syzygy1]

I think it should be PvNode and RootNode, because they are defined as constants.

Of course pvExact is just as much a constant, except that its constant value can be determined only halfway the function.

And then there is "const PieceToHistory* contHist[]" which should perhaps be ConstHist[] because of the "const". In the next line, "Move countermove" could become "const Move CounterMove".

If we want to "fix" this, maybe someone should first draw up a guideline for capitalisation (and for const usage).

Btw. I don't think the usage of "const" for local variables improves optimisation. Its only role is to catch programming errors.

[jerrydonaldwatson]

@mcostalba Thanks a lot for that, now I understand that one.
@syzygy1 Yes, PvNode and RootNode may be a better choice. I just noticed a small inconsistency which can be remedied in this way.

I've edited the table to reflect these comments.

[mcostalba]

@syzgy1 there is a difference between a constant and a variable that happens to be used as constant in a specific function.

PvNode is a real constant, instead history tables of course are not, they are just not changed in _some_ functions.

[mcostalba]

To be more clear, we can define constant as _compile time constant_:

• If name refers to a template parameter, a compile time constant, a global variable, a class or struct, a namespace, an enum type then the first char is uppercased

• In all the other cases the name first char is lowercased

[syzygy1]

Then it might make sense to use constexpr for PvNode?

[syzygy1]

Indeed constexpr bool PvNode compiles correct, whereas constexpr bool rootNode does not.

[mcostalba]

Yes, constexpr could be a good code's self-documenting tool in this case.

[gonlem]

Correcting a supposed typo in a method name.

Current name | Proposed rename | Rationale
-- | -- | --
kingAdjacentZoneAttacksCount | kingAdjacentAttacksCount | The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop | LongDiagonalBishop | Slightly more accurate definition in my opinion.
TrappedBishopA1H1 | TrappedCornerBishop | Slightly more descriptive name in my opinion.
unstablePvFactor | unstableBestMoveFactor | Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode | RootNode | Consistency: we already have PvNode.
is_KBPsKs(...) | is_KBPsK(...) | This method name is inconsistent with the related ScalingFunction "ScaleKBPsK" (see line 71 & 168 of material.cpp).

[protonspring]

Instead of TrappedCornerBishop. . . Just. . "CorneredBishopA1" etc. Sorry, i cant edit the table and i am on my phone.

[pb00068]

I suggest to rename pinnersForKing into pinnersOnKing because a 'pinner' actually pins a piece on a king, thus 'on' may be more correct than 'for'.

[xoto10]

Current name | Proposed rename | Rationale
-- | -- | --
kingAdjacentZoneAttacksCount | kingAdjacentAttacksCount | The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop | LongDiagonalBishop | Slightly more accurate definition in my opinion.
TrappedBishopA1H1 | TrappedCornerBishop | Slightly more descriptive name in my opinion.
unstablePvFactor | unstableBestMoveFactor | Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode | RootNode | Consistency: we already have PvNode.
is_KBPsKs(...) | is_KBPsK(...) | This method name is inconsistent with the related ScalingFunction "ScaleKBPsK" (see line 71 & 168 of material.cpp).
pinnersForKing | pinnersOnKing | position.h/.cpp
v | sc | line 115 in psqt.cpp

[Rocky640]

Why not simply
pinnersForKing[WHITE] => pinners[BLACK] (pieces which belongs to black side)
pinnersForKing[BLACK] => pinners[WHITE] (pieces which belongs to white side)

[syzygy1]

@Rocky640
Because pinnersForKing[WHITE] contains both white and black pieces.

[Rocky640]

I'm quite sure that pinnersForKing[WHITE] contains only some black pieces.
However blockersForKing[WHITE] contains both white and black pieces.

[syzygy1]

You are right, I am wrong :-)

[vondele]

Current name | Proposed rename | Rationale
-- | -- | --
kingAdjacentZoneAttacksCount | kingAdjacentAttacksCount | The name is already very long and the word "Zone" does not aid understanding.
LongRangedBishop | LongDiagonalBishop | Slightly more accurate definition in my opinion.
TrappedBishopA1H1 | TrappedCornerBishop | Slightly more descriptive name in my opinion.
unstablePvFactor | unstableBestMoveFactor | Slightly more accurate definition in my opinion, after all this does not measure changes to the PV apart from the best move itself.
rootNode | RootNode | Consistency: we already have PvNode.
is_KBPsKs(...) | is_KBPsK(...) | This method name is inconsistent with the related ScalingFunction "ScaleKBPsK" (see line 71 & 168 of material.cpp).
pinnersForKing | pinnersOnKing | position.h/.cpp
v | sc | line 115 in psqt.cpp
InCheck| inCheck| in qsearch, needed after turning it from a template param to a local variable

[mcostalba]

@vondele thanks for the update.

Please note that rootNode is correct because is defined as;

const bool rootNode = PvNode && ss->ply == 0;

and is not a compile time constant (instead PvNode it is):

const bool PvNode = NT == PV;

[snicolet]

My two cents:

• I like Alain's suggestion of pinnersForKing -> pinners
• InCheck -> inCheck is done
• I would do v -> score in line 115 of psqt.cpp
• all the other renaming proposals in Joost table are fine with me

[snicolet]

And I would propose contempt -> optimism everywhere.

[snicolet]

Implemented all (most) of the suggestions so far in this commit: 96362fe3df141eeead4bdb863d2bb2d891886abf

[Rocky640]

in timeman.cpp line 93 and 100, fix 2 comments:

// WARNING: Given npms (nodes per millisecond) must be much lower then
=>Given npmsec
// Convert from millisecs to nodes
==> milliseconds

[snicolet]

In Movepicker.cpp, rename the goto label:
goto begin_switch => goto top

[syzygy1]

I noticed that my constexpr suggestion was committed.
Should we now mark all compile-time local constants as constexpr (and verify that just those start with a capital)?

[vondele]

@syzygy1 I think that is a reasonable thing to do.

[jerrydonaldwatson]

The comment on line 378 of evaluate.cpp:
// Bonus for aligning rook with with enemy pawns on the same rank/file

should be amended to remove the double word "with".

[Rocky640]

The comment on line 695 of evaluate.cpp:
} // rr != 0

should be
} // w != 0

[xoto10]

thread.cpp line 120 "... launched threads will ..."

[Rocky640]

evaluate.cpp line 813
```
// Endings where weaker side can place his king in front of the enemy's
// pawns are drawish.

could be replaced with

// Endings where weaker side can place his king in front of at least one enemy
// pawn are drawish.

```

[syzygy1]

This comment has been incorrect now for a couple of years:
https://github.com/official-stockfish/Stockfish/blob/e408fd7b10503a9114962cb5972abd9957bc67c2/src/types.h#L260-L268
Nowadays, the least significant 16 bits are used to store the middlegame value and the upper 16 bits are used to store the endgame value.

[Rocky640]

In timeman.cpp

  // move_importance() is a skew-logistic function based on naive statistical
  // analysis of "how many games are still undecided after n half-moves". Game
  // is considered "undecided" as long as neither side has >275cp advantage.
  // Data was extracted from the CCRL game database with some simple filtering criteria.

This comment had been there since sf 2.3.1 and possibly before !
In sf 2.3.1, the move importance was encoded in a big array
A formula was introduced in sf 5 and the curve looks different.
Current values in sf9 are still quite similar to sf5.

I'm not sure how to "fix" that comment, but for sure, it is not accurate anymore.

[Matt14916]

The timeman.cpp move importance comment was originally left in after the fit so the origin of the fit and parameters would be documented, I think. The current values of the parameters are probably outside the original (questionable) error bars, but the history is probably good to mention, updated in some way ("originally based on" perhaps).

Since that area was brought up, in timeman.cpp, XScale and XShift should probably be named PlyScale and PlyShift or something similar. They are characteristic ply values for the function. The current names just reflect how the fitting was done and should have been chosen better originally.

[Rocky640]

@Matt14916 thanks for your explanations. Yes "originally based on..." could help. However, the explanation is not clear anyway, I would not know how to reproduce this original experiment even if I would have the original data (or some new one...) And from now on, all we will have there are tuned values.

There might be something to improve SF strength here. Maybe with the new contempt, the avg game length has changed. Also I wonder if the avg # plies should be different according to time control.

[jerrydonaldwatson]

@Rocky640
I also think there is room for improvement here. Another aspect is that Kai Laskos posted some data on Talkchess ( www.talkchess.com/forum/viewtopic.php?t=66821) which suggested that opening moves were less important than middlegame moves. In line with this, I tried to change the curve to use less time in the opening, and got two yellows:

http://tests.stockfishchess.org/tests/view/5aac657a0ebc5902975929e7
http://tests.stockfishchess.org/tests/view/5aace5ab0ebc590297592a65

[xoto10]

@Rocky640 @jerrydonaldwatson
Speaking of time management, one thing I have wondered about is when sf gets down to playing on increment. It seems to me that the remaining time typically gets too low, and then there is no flexibility to use 3x or 4x increment (or more) on a tricky move as it normally would. I'm not sure how to adjust it, but I think there might be a benefit in using very slightly less time when it is running out so that remaining time levels out at say 60s when playing with a 10s increment instead of levelling out at 20s or less.

[protonspring]

This has bit me a few times. EVERYWHERE in the code, BLACK means the side playing the black pieces and WHITE means the side playing the white pieces, except for pawnsOnSquares. It is a bit disorienting to see e->pawnsOnSquares[Us][BLACK] or [WHITE], but here it means the color of squares NOT the color of the pieces.

https://github.com/protonspring/Stockfish/tree/ps_lightdark

[Rocky640]

In bitboard.cpp, line 141, there are unnessary spaces in the following enum
Direction RookDirections[] = { NORTH, EAST, SOUTH, WEST };

[Rocky640]

in movepick.h, line 120 some extra spaces

MovePicker(const Position&, Move, Depth, const ButterflyHistory*,  const CapturePieceToHistory*, Square);

[Rocky640]

in evaluate.cpp line 736, the comment had not been updated

    // Find the safe squares for our pieces inside the area defined by
    // SpaceMask. A square is unsafe if it is attacked by an enemy
    // pawn, or if it is undefended and attacked by an enemy piece.
    Bitboard safe =   SpaceMask
                   & ~pos.pieces(Us, PAWN)
                   & ~attackedBy[Them][PAWN];

I suggest
// Find the available squares for our pieces inside the area defined by SpaceMask

[Rocky640]

In threads,cpp, line 120 replace wil with will

[snicolet]

pawns.cpp, unnecessary parentheses there:
if ((shift<Down>(theirPawns) & (FileABB | FileHBB) & BlockRanks) & ksq)

[protonspring]

In pawns.cpp, there is a Bitboard called "supported," but it doesn't include pawns that are supported. Instead, it includes the pawns that support. I think "support," or "supporters" better reflects what this Bitboard is doing.

[shooreek]

As it was already spotted by @Rocky640 few comments ago:

In threads,cpp, line 120 replace wil with will

"threads wil go immediately to sleep" -> "threads will immediately go to sleep"

[syzygy1]

In the Thread class, the nmp_ply and nmp_odd members should be renamed nmpPly and nmpOdd.

In the same class, the PVIdx and PVLast members should probably be renamed pvIdx and pvLast. See e.g. the tbHits member.

In that case, the TBRank and TBScore members of the RootMove struct should be renamed tbRank and tbScore.

In line 976 of tbprobe.cpp, comrpession should be compression (simple typo in a comment).

[syzygy1]

In position.h, from_to_bb should be fromToBB.

[snicolet]

• In has_game_cycle(), to and from should be s1 and s2
• trailing spaces in line 1003 of search.cpp

[jerrydonaldwatson]

In the comment here:
https://github.com/official-stockfish/Stockfish/commit/3d6995eae8039b2bf4141cbc02d87d5a6c2a1905

we need a space between "//" and "Increase"

[protonspring]

In pawns.cpp, the three basic pawn penalties (Isolated, Backward, and Doubled) area clearly self documenting and don't need their own special comments/spaces. . I propose:

//Pawn Penalties
constexpr Score Isolated = S(13,16);
constexpr Score Backward = S(17, 11);
constexpr Score Doubled = S(13,40);

[snicolet]

Tabs and spaces in line 1167 of search.cpp

[Rocky640]

line 779 of evaluate.cpp, there is an extra space
-136 ; --> -136;

[syzygy1]

https://github.com/official-stockfish/Stockfish/blob/a834bfe8332adcb0dfc1fd280f1f9d8bbce86266/src/search.cpp#L953-L955
Since PawnValueEg * (depth / ONE_PLY) is of type Value, the cast to Value is superfluous and should be removed.

[protonspring]

In pawns.cpp, lines 224 and 227, the default value for ourRank and theirRank should probably be RANK_1 instead of 0.

[syzygy1]

@protonspring
In that case, it seems that ourRank and theirRank should also be of type Rank instead of int.

[protonspring]

I agree with that.

On Sat, Jun 23, 2018 at 4:31 PM, syzygy1 [e-mail] wrote:

@protonspring https://github.com/protonspring
In that case, it seems that ourRank and theirRank should also be of type
Rank instead of int.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/official-stockfish/Stockfish/issues/1426#issuecomment-399714775,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AcVnSNeFBvJGbS3kCxcGbQ44SGV3FWu0ks5t_sHegaJpZM4SR1Zw
.

--
Michael T. Whiteley
mobile: [phone]

[Rocky640]

in evaluate.cpp
// Bonus for king attacks on pawns or pieces which are not pawn-defended

should be something like
// Bonus for king attacks on pawns or pieces which are not strongly protected

or more simply, remove this comment.

[Rocky640]

in evaluate.cpp
the Overload bonus and the following comment
// Bonus for overload (non-pawn enemies attacked once or more and defended exactly once)

Technically, a piece is "overloaded" (or "overworked") if it has more than one defensive duty.
For example, it is needed to defend 2 or more squares, and moving to one such square would leave the other poorly defended. It might also be for example a rook which is needed on the back rank otherwise a mate will follow, and thus cannot defend squares along the file.

This is not exactly what we are computing here.

Suggestions? I was thinking of Pressured, or Tied

[mcostalba]

Drop

include

from evaluate.h

[Rocky640]

In position.h line 479
if (!can_castle(WHITE) && !can_castle(BLACK))
can be replaced with
if (!can_castle(ANY_CASTLING))
as we do in search.cpp line 650 and 1670

[Rocky640]

the name "can_castle", although short and sweet, is misleading in 2 ways
a) it returns an int, not a bool
b) it is only about remaining castling rights, not about the legality of a next move castling

Maybe the longer "castling_rights" would leave no surprise.

Also note that
int can_castle(CastlingRight cr) const;
is always used as a bool

int can_castle(Color c) const;
would always be used as a CastlingRight (an accessor to record the current castling right for a given color) after the change proposed in the post just above

[Rocky640]

In misc.cpp line 231
int get_group(size_t idx) {

this was taken from Texel code

However, in SF, nowhere else do we use names such as get_ or find_

Maybe something like bestGroupForThread

[protonspring]

Line 577 of evaluate.cpp says: "Keep only the squares which are not completely unsafe." This could be much more clear with something more like: "Keep only the squares which are relatively safe." or similar.

[snicolet]

• pawns.cpp, lines 35-37: fix spaces, use alphabetical order
• evaluate.cpp: better comments for king tropism

[Rocky640]

material.cpp line 62 there is an extra space before ==
&& pos.count<QUEEN>(us) == 1

[ghost]

https://github.com/official-stockfish/Stockfish/pull/1755

[syzygy1]

In the README, the links to the chess programming wiki should be updated to https://www.chessprogramming.org/Main_Page and https://www.chessprogramming.org/Stockfish

[syzygy1]

between_bb() is still used, so BetweenBB[][] too. Or am I missing something?

[Rocky640]

You are right. I will delete my comment !

[Rocky640]

In evaluate .cpp, we can replace
nonPawnEnemies = pos.pieces(Them) ^ pos.pieces(Them, PAWN);
with
nonPawnEnemies = pos.pieces(Them) & ~pos.pieces(PAWN);

Because under the hood we have this for the first
byColorBB[c] ^ (byColorBB[c] & byTypeBB[pt])

And this for the proposed change, so less instructions for the same result
byColorBB[c] & ~byTypeBB[pt]

[eduherminio]

from PR #1774
In search.cpp:
// Singular extension search (~60 Elo). If all moves but one fail low on a
// search of (alpha-s, beta-s), and just one fails high on (alpha, beta),
// then that move is singular and should be extended. To verify this we do
// a reduced search on on all the other moves but the ttMove and if the
// result is lower than ttValue minus a margin then we will extend the ttMove.

[pb00068]

line 913:
&& !excludedMove // Recursive singular search is not allowed

The comment is misleading, since recursive singular search is allowed and wanted by design,
we just not want a vicious cycle at the same ply, so maybe better:

&& !excludedMove // needed to avoid a vicious circle

[pb00068]

The term rBeta is used twice in search:

• in probcut where it means: raised beta
• in singular search where it stands for: reduced beta

This confuses me every time I glance at this variables and prevents me to memorize
this 2 different techniques correctly (do we raise or reduce beta in probcut? and what on singular search? ).
Using explicit variable names here help's IMO to keep things clear in mind:
Probcut -> raisedBeta
Singular Extension -> reducedBeta

[syzygy1]

https://github.com/official-stockfish/Stockfish/blob/982fd9c8bce85d4cd8dca32759587d7606385e0e/src/syzygy/tbprobe.cpp#L1110-L1111

There is a c missing in 'aquire', and I think it would be easier to read if "to avoid a thread reads" is changed to "to avoid that a thread reads".

Oh, and in line 1113 exsist should be exist.

[protonspring]

There are some TABS in search.cpp that should probably be converted to spaces.

lines 451, 1189, 1190, and 1191.

[mcostalba]

Time for some roll up. Is there someone interested in flushing this log?

After a possible cumulative patch is applied I will open a new thread. Eventually to avoid threads to become too long and to allow easier understanding of current status, we should close the old one and reopen a new one every time the corresponding cumulative formatting patch is applied.

[protonspring]

I can. Should i make a patch that incorporates all of them?

On Sun, Dec 9, 2018, 1:21 AM Marco Costalba <[email protected] wrote:

Time for some roll up. Is there someone interested in flushing this log?

After a possible cumulative patch is applied I will open a new thread.
Eventually to avoid threads to become too long and to allow easier
understanding of current status, we should close the old one and reopen a
new one every time the corresponding cumulative formatting patch is applied.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/official-stockfish/Stockfish/issues/1426#issuecomment-445519400,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AcVnSNiZ9e-HH5wTZObSHGSvQw_NVTRxks5u3MgUgaJpZM4SR1Zw
.

[protonspring]

I made a PR with all of the changes I could find in this issue.

https://github.com/official-stockfish/Stockfish/pull/1861

If you have additions, please either post them in the PR so they can be added, or wait for another (clean) RENAMING issue to be added.

[mcostalba]

@protonspring great! I am going to close this one and reopen a new one