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

Retire KingDanger array #786

Closed
wants to merge 4 commits into from
Closed

Conversation

hxim
Copy link

@hxim hxim commented Sep 5, 2016

Replaced KingDanger array with direct calculation. Function is designed so that
a) in interval 0..QuadUnits quadratic function f1(x) = a * x * x is used
b) in interval QuadUnits..MaxUnits linear function f2(x) = x - b is used

Parameters a and b are must be a = 1 / (QuadUnits * 2) and b = QuadUnits / 2
because we need for x := QuadUnits both

1.) derivations same
f1'(QuadUnits) = f2'(QuadUnits)
=>
2 * a * QuadUnits = 1
=>
a = 1 / (QuadUnits * 2)

2.) values same
f1(QuadUnits) = f2(QuadUnits)
=>
a * QuadUnits * QuadUnits = QuadUnits - b
=>
b = QuadUnits - a * QuadUnits * QuadUnits
=>
b = QuadUnits / 2

STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 41445 W: 7762 L: 7677 D: 26006

LTC:
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 70258 W: 9381 L: 9326 D: 51551

bench 8507177

@Rocky640
Copy link

Rocky640 commented Sep 5, 2016

So it's about the same shape as before.
But was it necessary to multiply all the parameters in evaluate.cpp by 11 ?
Could this be done only at the end of the calculation instead ?

@hxim
Copy link
Author

hxim commented Sep 5, 2016

@Rocky640 Yes, shape is same as before. I multiplied the parameters because it allows to have 1 less parameters in formula (now we have only 2 parameters - MaxUnits 2543 and QuadUnits 1786 instead of 3 parameters before MaxSlope 322, Peak 47410 and Weight 268 / 7700). Also i find nice that one attackUnit equals Value(1) of middlegame score in the linear part of function (it is less in quadratic part)

@snicolet
Copy link
Member

snicolet commented Sep 5, 2016

So the function is a little bit like a poor man's approximation of a sigmoid with only polynomials of degree <= 2. The question is how we could further simplify it in line 483-484 after the patch is commited, if any. Maybe just continue the red function up to the green level ?

@mcostalba
Copy link

mcostalba commented Sep 5, 2016

First of all, congratulations for this patch: it is not easy to have something working in king safety, especially such drastic idea is a really surprise that is proved not weaker than master.

If this patch would have been a [0, 5] or even a [0, 4] I would have gladly applied. But I am a bit dubious to totally replace a more or less easy to understand, easy to tweak table with something that is more difficult and less immediate. Your patch it is a simplification just because it removes KingDanger[] init code, but actual running code in evaluate_king() is not easier than current master, actually it is even a bit more complex, so I am wondering if it makes sense to replace something easy and well proven with something totally new just because it is not weaker.

Can you please run a SPRT[0, 4] at LTC?

snicolet added a commit to snicolet/Stockfish that referenced this pull request Sep 5, 2016
Based on work by "hxim" in
official-stockfish/Stockfish#786 , but using
only the red quadratic function and not the linear part.
@hxim
Copy link
Author

hxim commented Sep 5, 2016

@mcostalba sure test submited. I dont consider this change drastic because the calculation is basically same as before only array is not used - thats why it passed non regression test i think. Also i would think new code is easier to understand because it is immediately visible in code how attackUnits quantity is transformed into score. But I agree in the future it may be more difficult to tweak or replace with some more complex function than with KingDanger array.

@snicolet
Copy link
Member

snicolet commented Sep 5, 2016

Just out of curiosity, I have pushed a test to see if the blue part of the graph is necessary (the test uses the same quadratic as the red part of the graph, up to the green level): http://tests.stockfishchess.org/tests/view/57cdcecb0ebc59030fbe4f2c

@hxim
Copy link
Author

hxim commented Sep 5, 2016

@snicolet removing blue part is same as seting MaxSlope to infinity in current code and will fail quickly i think

@snicolet
Copy link
Member

snicolet commented Sep 5, 2016

I should have been more cautious with wording: I didn't mean adding a step at the end of the red function to the green level, but rather continue the red function to the right, up to the point where it reaches the green level.

I did a bench run where I computed the two versions of the king danger

k = using red + blue + green
k' = using red + red continuation + green

and then adding dbg_mean_of(abs( k - k')) resulted in:

Total 4045734 Mean 1.51005

meaning that k and k' differ by about 1.51 on average (maybe because the blue part of the graph is relatively rarely used, in only 4% of all the evaluation calls).

@hxim
Copy link
Author

hxim commented Sep 5, 2016

@snicolet i understand what you mean but if you make calculation of KingDanger array items you will see it is the same as removing MaxSlope parameter. That is why i would be surprised if it passed because i think this parameter was tuned in the past along with other parameters in king safety.

@snicolet
Copy link
Member

snicolet commented Sep 5, 2016

@hxim: I don't know, let's just wait for the test result :-)

In fact your simplification and the discussion we are having just makes me realize that the version with only the red+green parts if equivalent (same bench) to

int A = attackUnits > 0 ? attackUnits : 0;
score -= make_score(std::min( A * A / 3572 , 1650), 0);

which proves that the king danger malus can never be more than two bishops (two bishops are 1652 exactly): honestly, I had never been aware of this by reading the current master code, so thanks for your insightful patch !!

So in fact my patch could almost be rewritten as :

int A = attackUnits > 0 ? attackUnits : 0;
score -= make_score(std::min( A * A / 3572 , 2 * BishopValueMg), 0);

@joergoster
Copy link
Contributor

@hxim Sorry to say, but I fail to see the real benefit. A simple lookup is much easier to follow than your formula, imho. And the calculation of KingDanger array comes with no cost because is done at startup. Just my noobish 2 cents, of course.
@snicolet Thank you for the nice graph and the explanation.

@mcostalba
Copy link

@hxim It seems SPRT[0, 4] test failed and this patch really boils down to a parameter tweak: you replace KingDanger[] values with another set of parameters.

So the bad news is that I am going to close this PR.

The good news is that the road is open and eventually tuning/tweaking your formula will result in a real improvement compared to current master and I will gladly apply.

@mcostalba mcostalba closed this Sep 6, 2016
snicolet added a commit to snicolet/Stockfish that referenced this pull request Sep 7, 2016
Retire KingDanger array

Rescales the king danger variables in evaluate_king() to
suppress the KingDanger[] array. This avoids the cost of
the memory accesses to the array and simplifies the non-linear
transformation used.

Full credits to "hxim" for the seminal idea and implementation,
see pull request #786.
official-stockfish/Stockfish#786

Passed STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 9649 W: 1829 L: 1689 D: 6131

Passed LTC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 53494 W: 7254 L: 7178 D: 39062

Bench: 7789537
@snicolet snicolet mentioned this pull request Sep 7, 2016
mcostalba pushed a commit that referenced this pull request Sep 16, 2016
Rescales the king danger variables in evaluate_king() to
suppress the KingDanger[] array. This avoids the cost of
the memory accesses to the array and simplifies the non-linear
transformation used.

Full credits to "hxim" for the seminal idea and implementation,
see pull request #786.
#786

Passed STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 9649 W: 1829 L: 1689 D: 6131

Passed LTC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 53494 W: 7254 L: 7178 D: 39062

Bench: 6116200
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.

None yet

5 participants