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

Simplify SEE calculation (no X-Ray) #365

Closed
wants to merge 2 commits into from

Conversation

Rocky640
Copy link

Passed STC
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 31885 W: 6055 L: 5953 D: 19877

And LTC
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 76309 W: 12017 L: 11981 D: 52311

Bench: 9159664

Same version which passed the tests, but with dead code removed.
See also next pull request for a possibly faster version.

Do not use x-ray in SEE.
Bench #  9159664
Master # 6716940
Remove dead code. No functional change
@mcostalba
Copy link

mcostalba commented Jun 13, 2015 via email

@glinscott
Copy link
Contributor

This is quite interesting that it passed. It's not a simplification though, it does break the idea behind SEE. Now, the fact that the full SEE doesn't seem to be necessary is an idea worth testing some more! But I agree with Marco that this patch should not go in as a simplification.

RE: approving the test runs - I generally just make sure that the code is not malicious. I don't usually check the SPRT bounds, but perhaps we should have an alert if they are different that the usual.

@Stefano80
Copy link
Contributor

@mcostalba: I dont see your point. If Rocky640 would just change the name of the function to, say, ROCKY_SEE, would be ok for you? Your comparison to "simplify castling, removing long castling", or "simplify pawns, removing en-passant" does not hold since SEE is not part of chess rules.

@Rocky640
Copy link
Author

OK, let's call this LazySEE.

This LazySEE is just a bit more blind than SEE
(which is already blind to whatever may happen on other squares, in between moves, capture of some of the attackers/defenders of the square, checks, pins, double threat, overloading, and so on)

@mcostalba
Copy link

What is the advantage of lazy see? Two lines less of code? Oh well..

You guys are totally missingg the point : this is not a simplification,
this is breaking SEE for absolutely no reason.

If your patch would prove stronger maybe, after renaming SEE, you could
have a point, but in this case is just breaking well known and reference
stuff for free.

On Saturday, June 13, 2015, Rocky640 notifications@github.com wrote:

OK, let's call this LazySEE.

This LazySEE is just a bit more blind than SEE
(which is already blind to whatever may happen on other squares, in
between moves, capture of some of the attackers/defenders of the square,
checks, pins, double threat, overloading, and so on)


Reply to this email directly or view it on GitHub
#365 (comment)
.

@zamar
Copy link

zamar commented Jun 13, 2015

I agree with Marco.

A change that removes code lines but breaks the underlying logic is not a simplification. It's an interesting result though...

I'm closing the request because the patch is not committable, it would need to pass [0,4] to be committed.

@zamar zamar closed this Jun 13, 2015
@Stefano80
Copy link
Contributor

Hi,

I dont have a really strong opinion on the point, but I have a question. Why have the following patches

aaf1732
ef4d89c
20e9289

been commited as simplification, and the LazySee patch not? In particular the second one surprises me, i would have considered it a parameter tweak.

I really just want to understand how you guys think about this simplification stuff.

@mcostalba
Copy link

SEE is a classic computer chess algorithm, well described in literature and
not invented by us: we have a reference implemantation in SF that
interested people can look up and they can be sure they are looking at the
real thing, not a trick/tweak. This is important because SF aims to be a
reference engine.

Patches you refer to are just tweaks, they don't implement any classical
algorithm and have been written by SF people in endless number of
iterations, and will be rewritten another endless times.

On Sat, Jun 13, 2015 at 6:03 PM, Stefano Cardanobile <
notifications@github.com> wrote:

Hi,

I dont have a really strong opinion on the point, but I have a question.
Why have the following patches

aaf1732
aaf1732
ef4d89c
ef4d89c
20e9289
20e9289

been commited as simplification, and the LazySee patch not? In particular
the second one surprises me, i would have considered it a parameter tweak.

I really just want to understand how you guys think about this
simplification stuff.


Reply to this email directly or view it on GitHub
#365 (comment)
.

@Stefano80
Copy link
Contributor

Ok, this is an understandable argument.

So, can we agree that "A simplification is a patch which removes code and does not break any algorithm for which SF claims to have a reference implementation"? This is just for me for the future...

@zamar
Copy link

zamar commented Jun 13, 2015

What is a simplification and what is not? This is always somewhat question
of taste. At least the following aspects to be considered:

  • Number of code lines added/removed
  • Logical complexity of code
  • Readability of code
  • How well the code follows the existing chess programming standards.

On Sat, Jun 13, 2015 at 6:22 PM, Stefano Cardanobile <
notifications@github.com> wrote:

Ok, this is an understandable argument.

So, can we agree that "A simplification is a patch which removes code and
does not break any algorithm for which SF claims to have a reference
implementation"? This is just for me for the future...


Reply to this email directly or view it on GitHub
#365 (comment)
.

@Rocky640 Rocky640 deleted the SEE-1 branch August 20, 2015 01:00
Rocky640 referenced this pull request in pb00068/Stockfish Apr 12, 2016
bench 7737071
niklasf pushed a commit to niklasf/Stockfish that referenced this pull request Jun 7, 2017
Tweak evaluation parameters for racing kings
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