Improve commentary about DEPTH constants #5205

dubslow · 2024-05-01T13:06:22Z

Also "fix" movepicker to allow depths between CHECKS and NO_CHECKS, which makes them easier to tweak (not that they get tweaked hardly ever) (This was more beneficial when there was a third stage to DEPTH_QS, but it's still an improvement now)

no functional change

github-actions · 2024-05-01T13:06:56Z

clang-format 17 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/mantic/clang-format-17.

(execution 9154229307 / attempt 1)

peregrineshahin · 2024-05-02T04:19:45Z

src/types.h

-    DEPTH_NONE = -6,
-
-    DEPTH_OFFSET = -7  // value used only for TT entry occupancy check
+  // In QS, with depth <= 0, we search only "tactical" moves to fight the


Fighting against the horizon effect is only a subproduct of qsearch, the idea is to have a more stable static evaluation of the position such that there are no tactical solutions left, that could render the static evaluation useless.

i.e. if our evaluation function is as good in evaluating the tactical positions as it is good for quiet positions, we don't need no qsearch at all. you can just return eval.

the idea is to have a more stable static evaluation of the position such that there are no tactical solutions left, that could render the static evaluation useless.

this is part of what is meant by "the horizon effect" no?

we search only "tactical" moves to fight the horizon effect.

well, my nitpick is that searching (only "tactical" moves) does not fight against the horizon effect and hints that if you try any other moves you will get horizoned, which is not accurate, you will get an explosion and not neccessarly a horizon effect.

rather I find something like "because the horizon effect is more detrimental in the tactical positions we limit searching to them, otherwise we trust the evaluation function"
more to the point and less misleading.

dubslow · 2024-05-04T13:22:22Z

I've pushed a new version with cj's constant names, and expanding the comment based on peregrine's concerns

vondele · 2024-05-05T09:46:18Z

Note really my taste, like reading the comments I run into terms that I find non-standard unclear.

cutoff priority
flat depth
_NORMAL phase
volatile/tactical moves
static eval stability

I don't think prexisting comments were wrong or misleading, so I would probably not merge this one, as per previous suggestion by @Disservin

Disservin · 2024-05-05T09:52:37Z

I don't have something against new comments which explain logic.. reordering of words is also mostly useless because it also very much a personal choice.

I do think that the depth types could use some explanation though. DEPTH_NONE i.e. explain why it is -6 ? and how is the relation of depth offset ? is it always supposed to be one lower than none depth? then maybe rewrite that as such

// _NONE is used throughout the codebase as the non-value, is perhaps a bit useless to write
none is used as non ok? :P

dubslow · 2024-05-05T13:44:02Z

cutoff priority

that's from existing master, and wasn't my contribution. not related to this PR, tho perhaps you think this PR is a good place to fix this issue with master?

flat depth

if you have another idea in mind to succinctly describe "we dont store qs depth in tt other than 0 and -1", im all ears.

_NORMAL phase

this was cj's suggestion for standard QS behavior, again, if you have another idea im all ears.

edit: ah, perhaps "stage" is more standard than "phase"? that's my bad

volatile/tactical moves; static eval stability

these werent in my original PR, and id be happy to delete them

// _NONE is used throughout the codebase as the non-value, is perhaps a bit useless to write
none is used as non ok? :P

perhaps "the absence of a value"?

explain why it is -6

tbh it's presently only -6 because it wasn't changed when the _RECAPTURES stage was removed, and since this PR is supposed to be focused on comments only i hadn't wanted to include that change here. should i?

dubslow · 2024-05-05T14:25:34Z

In addition to tweaks per comments, I've also made a further noticeable change to the DEPTH comments, since I'd just realized I was still missing half the interactions despite trying actively to document the interactions. (Good questions, Disservin, they got me to notice what I'd missed.)

I've left separate commits for reviewing, but can easily squash at any time (e.g. when accepted)

peregrineshahin · 2024-05-05T16:22:27Z

src/search.cpp

@@ -1435,8 +1435,11 @@ Value Search::Worker::qsearch(Position& pos, Stack* ss, Value alpha, Value beta,

    assert(0 <= ss->ply && ss->ply < MAX_PLY);

-    // Decide the replacement and cutoff priority of the qsearch TT entries
-    ttDepth = ss->inCheck || depth >= DEPTH_QS_CHECKS ? DEPTH_QS_CHECKS : DEPTH_QS_NO_CHECKS;
+    // Decide the replacement and cutoff priority of qsearch TT entries. Note that we don't


Perhaps this is better.. I made the priority comment before a while

+ // ttDepth decides when to replace and when to TT cutoff for the qsearch TT entries. - // Decide the replacement and cutoff priority of qsearch TT entries.

dubslow · 2024-05-05T19:19:34Z

peregrine's most recent comment got my brain going once again, and ive now re-overhauled all the related comments. qs generals is at qs function, ttdepth comment doesn't mention the uses of ttdepth, and the DEPTH comments now reflect that these constants are infact only used for tt entries, which somehow i hadn't realized before today. i think the present form is considerably improved over the first version of the PR (and thus an even bigger improvement over master).

I've also included a suggested renaming of DEPTH_NONE to DEPTH_UNSEARCHED, since it in fact isn't used all over the codebase like my first draft implied, it's used exclusively for ttentries, so the new name is much more accurate. ive also suggested DEPTH_OFFSET be expanded to DEPTH_ENTRY_OFFSET, which is more accurate but longer, and im not sold that that length is justified by the accuracy. thoughts?

would squashing be easier to review?

dubslow · 2024-05-05T20:05:41Z

I went ahead and squashed onto master. For the commit history that I've gone thru today to reach the present point see here: master...dubslow:Stockfish:qsDepthStagesTouchupOld

Turns out they're only related to ttdepth, so some reorganization of related comment chunks is done as well. Rename the constants too. (Also "fix" movepicker to allow depths between CHECKS and NORMAL, which makes them easier to tweak, not that they get tweaked hardly ever. This was more beneficial when there was a third stage to DEPTH_QS, but it's still an improvement now) no functional change

Disservin

after the changes i think we can merge it

Disservin · 2024-05-19T14:40:33Z

src/tt.cpp

@@ -30,6 +30,9 @@

 namespace Stockfish {

+// Per commit f7b3f0e, we use `bool(depth8)` to test if an entry is occupied. However we also need to


i find linking to a specific commit a bit weird

Yes, on the other hand it does contain a lot more relevant detail. I thought best to keep comments short but leave a pointer to more should the reader be interested...

do you have alternatives in mind? not sure how best to do this

mh okay just had a look at the pr and it seems it had a bit more detail than the avg one so can stay, just dont want all comments to be cluttered with "got introduced with xyz"

perhaps it should say "per the details in commit f7b3f0e"?

though what is the comment doing at the save function ? it aint being tested there only in the probe, where the meaning is kinda self explanatory found = bool(tte[i].depth8) could point to the commit there

well i put the comment where we make heaviest use of DEPTH_ENTRY_OFFSET, since the comment is ultimately about that constant and why it exists. im happy to move it where you like

On further thought, based on your comments here, I reviewed the old commit and concluded that more of its message should have been code-comments to begin with, so added those and removed the direct reference to the commit.

Is the resulting comment sufficiently short?

src/search.cpp

dubslow · 2024-05-27T03:49:02Z

Thanks to all the comments, every single comment resulted in some improvement.

peregrineshahin reviewed May 2, 2024

View reviewed changes

Disservin added the no-functional-change label May 4, 2024

dubslow force-pushed the qsDepthStagesTouchup branch from aca4195 to b3dd845 Compare May 4, 2024 13:21

peregrineshahin reviewed May 5, 2024

View reviewed changes

dubslow force-pushed the qsDepthStagesTouchup branch 2 times, most recently from 35e5013 to af90b90 Compare May 5, 2024 20:05

dubslow changed the title ~~Explain DEPTH constants for ease of understanding~~ Improve commentary about DEPTH constants May 17, 2024

dubslow force-pushed the qsDepthStagesTouchup branch from af90b90 to d603a0c Compare May 17, 2024 04:07

Disservin reviewed May 19, 2024

View reviewed changes

dubslow added 2 commits May 19, 2024 11:07

(further tweaks)

00a4c80

(further tt tweaks)

05f90b6

Disservin added the to be merged Will be merged shortly label May 23, 2024

Disservin closed this in ed79745 May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve commentary about DEPTH constants #5205

Improve commentary about DEPTH constants #5205

dubslow commented May 1, 2024

github-actions bot commented May 1, 2024 •

edited

Loading

peregrineshahin May 2, 2024 •

edited

Loading

peregrineshahin May 2, 2024

dubslow May 2, 2024

peregrineshahin May 2, 2024

dubslow commented May 4, 2024

vondele commented May 5, 2024

Disservin commented May 5, 2024 •

edited

Loading

dubslow commented May 5, 2024 •

edited

Loading

dubslow commented May 5, 2024

peregrineshahin May 5, 2024

dubslow commented May 5, 2024 •

edited

Loading

dubslow commented May 5, 2024

Disservin left a comment

Disservin May 19, 2024

dubslow May 19, 2024

Disservin May 19, 2024

dubslow May 19, 2024

Disservin May 19, 2024

dubslow May 19, 2024

dubslow May 20, 2024 •

edited

Loading

dubslow commented May 27, 2024

		@@ -30,6 +30,9 @@

		namespace Stockfish {

		// Per commit f7b3f0e, we use `bool(depth8)` to test if an entry is occupied. However we also need to

Improve commentary about DEPTH constants #5205

Improve commentary about DEPTH constants #5205

Conversation

dubslow commented May 1, 2024

github-actions bot commented May 1, 2024 • edited Loading

peregrineshahin May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubslow commented May 4, 2024

vondele commented May 5, 2024

Disservin commented May 5, 2024 • edited Loading

dubslow commented May 5, 2024 • edited Loading

dubslow commented May 5, 2024

Choose a reason for hiding this comment

dubslow commented May 5, 2024 • edited Loading

dubslow commented May 5, 2024

Disservin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubslow May 20, 2024 • edited Loading

Choose a reason for hiding this comment

dubslow commented May 27, 2024

github-actions bot commented May 1, 2024 •

edited

Loading

peregrineshahin May 2, 2024 •

edited

Loading

Disservin commented May 5, 2024 •

edited

Loading

dubslow commented May 5, 2024 •

edited

Loading

dubslow commented May 5, 2024 •

edited

Loading

dubslow May 20, 2024 •

edited

Loading