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

Merge scale_factor() and mg/eg interpolation into initiative() #2710

Closed
wants to merge 5 commits into from

Conversation

xoto10
Copy link
Contributor

@xoto10 xoto10 commented Jun 3, 2020

Merging this code into one function puts all the endgame adjustment into initiative() instead of spreading it across initiative() and scale_factor() with the final interpolation in value(). This makes value() cleaner while allowing mixing of the ideas in the former initiative() and scale_factor() functions.

Future work: hopefully this will encourage devs to try using scale_factor for initiative calcs and/or initiative elements in the calculation of scale_factor, leading to more sophisticated eg valuation.

No functional change.

@MichaelB7
Copy link
Contributor

An excellent simplification. Nice work!

@snicolet
Copy link
Member

snicolet commented Jun 4, 2020

The following remarks are not judgements on the opportunity to commit the patch, I will let Joost take his decision on this point.

But if the patch is accepted, then:

a) the comment in line 760 is less clear than the two comments in lines 765 and 774 of master.

b) the trace feature is regressive compared to master (both the INITIATIVE and the TOTAL terms changed semantics) in the initial PR version of the patch. The fix would be to add the following code around line 758:

   
    mg += u;
    eg += v;

    if (T) 
    {
        Trace::add(INITIATIVE, make_score(u, v));
        Trace::add(TOTAL, make_score(mg, eg));
    }

Then use mg and eg in line 780 to calculate the tampered value, and suppress the INITIATIVE and TOTAL tracing calls in lines 784 and 851.

c) the name of the new initiative() function is now misleading.

@xoto10
Copy link
Contributor Author

xoto10 commented Jun 4, 2020

@snicolet
Yes, I thought about a) and c) and I wasn't sure of the best solution. I didn't really look at b) - good point! :)

@xoto10
Copy link
Contributor Author

xoto10 commented Jun 4, 2020

For completeness, I should point out that I started a test with an earlier version of this change:
https://tests.stockfishchess.org/tests/view/5eced39b75787cc0c05d97bb
xoto10/stockfish-xoto10@fb80957...655988c

This merged scale_factor() into initiative(), and adjusted the score accordingly, but returned the score to value() which still did the interpolation. This gave a rounding difference to master, and I believe was less accurate than master, this is why I moved the interpolation into initiative() as well. An alternative solution would be to return the initiative adjustment and the scale factor back to value(), and keep the interpolation in value(). Perhaps this is more in the spirit of the change I am trying to make, I think the initiative adjustment and the scale factor calculation overlap, and should be calculated by the same function.

@xoto10
Copy link
Contributor Author

xoto10 commented Jun 4, 2020

No, I think that last comment from me is wrong.

What I am trying to do here is combine initiative() and scale_factor() into one function. If we are making adjustments for pawnsOnBothFlanks (currently in initiative()) or for opposite_bishops() (currently in scale_factor()), these are all assessments of the amount of pressure we are applying to the opponent and I think they should be in the same function so that we can take advantage of any interplay or commonality.

The current trace output doesn't include scale_factor, but I think that is a bug, we should include the scale_factor adjustment in the trace output of the updated function and trace output of the total. I have pushed an update to do this.

@vondele
Copy link
Member

vondele commented Jun 8, 2020

@xoto10 I have been looking at this, but I'm not really convinced about this PR at this point. I guess my main difficulty is understanding why merging initiative and scalefactor makes things more clear or better. Obviously, the goals you mention in the initial comment are worthwhile.

@xoto10
Copy link
Contributor Author

xoto10 commented Jun 9, 2020

@vondele
I think I have 2 main points:

  • initiative() and scale_factor() overlap
  • scale_factor() is naming the implementation, not the job and the current code structure is more modular and more rigid than it needs to be

Initiative() and scale_factor() both adjust the eg value according to the pressure being applied by the stronger side, upwards or downwards.
For example, almostUnwinnable is used in initiative() to penalise poor endings, and opposite_bishops(), piece count, etc are used in scale_factor() to penalise poor endings. Pawn count is used in both functions. It doesn't seem right to do these things in 2 separate places. To me this is a code refactor to put related code into 1 place.

Connected to this is the kind of calculation. Currently initiative() adds an increment to the score, and scale_factor calculates a multiplier to apply to the score, and we choose which function to modify depending on which approach we want to try. And then we do an interpolation that uses the scale factor. But we could be using many different operations, perhaps in a complicated multistep process like we do when scoring kingdanger. I think that putting the 3 steps (add, scale, interpolate) into one function will help devs develop more sophisticated options. (For example, use initiative elements to influence the scale factor or use elements from either of those to influence the interpolation step. Add non-linear terms, etc.)
I think the comparison with kingdanger is apt, we don't have several different functions for linear calcs, king defence calcs, power 4 king calcs, we just have one function king(), and devs do any wild stuff they can think of in there to assess king danger. I think we should do the same here.

I don't have any particular preference for how we name the function, initiative(), pressure(), winnable(), adjust_and_merge() or just win(), there are many options and ultimately it's just a name. But I do think it should suggest "what" we're doing, not "how" we're doing it. I used "Winnable" below just to differentiate the old and new output.

Regarding the trace output, this OCB position illustrates the change made to the Initiative and Total outputs from trace:

position fen 4bk2/8/rB6/P1P5/1p5p/7P/8/3R3K w - - 5 53
eval
...
  Initiative |  ----  ---- |  ----  ---- | -0.03 -0.28
 ------------+-------------+-------------+------------
       Total |  ----  ---- |  ----  ---- |  1.73  0.64

Total evaluation: 0.56 (white side)
...
    Winnable |  ----  ---- |  ----  ---- | -0.03 -0.52
 ------------+-------------+-------------+------------
       Total |  ----  ---- |  ----  ---- |  1.73  0.40

Final evaluation: 0.56 (white side)

Master gives mg/eg values for Total of 1.73 and 0.64, and we interpolate between them to get ... 0.56, which isn't inside that range! Clearly there is a scale factor involved, but we don't say what it is, and reporting a final eval outside the mg/eg range for Total seems odd to me.
This patch gives Winnable values (formerly Initiative) that include the scale factor adjustment, so it reports a larger penalty, the eg value for Total is now 0.40 and the final eval of 0.56 does fall between the reported Total mg and eg values. We still don't see the scale factor, but we can now see the fully adjusted eg value, which seems like useful info to me.

@vondele vondele added the to be merged Will be merged shortly label Jun 9, 2020
@vondele vondele closed this in 9023090 Jun 9, 2020
@vondele
Copy link
Member

vondele commented Jun 9, 2020

Thanks!

MichaelB7 pushed a commit to MichaelB7/Stockfish that referenced this pull request Jun 13, 2020
Merging this code into one function `winnable()`.
Should allow common concepts used to adjust the eg value,
either by addition or scaling, to be combined more effectively.

Improve trace function.

closes official-stockfish#2710

No functional change.
@syzygy1
Copy link
Contributor

syzygy1 commented Jul 5, 2020

I like this patch a lot!

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

5 participants