Code review , and what is the next step ?

Discussion of chess software programming and technical issues.

Moderators: hgm, Rebel, chrisw

MahmoudUthman
Posts: 234
Joined: Sat Jan 17, 2015 11:54 pm

Code review , and what is the next step ?

Post by MahmoudUthman »

My search routine :

Code: Select all

int q_search(int alpha, int beta)
{
	if (!InCheck)
	{
		int stand_pat = Evaluate();
		if (stand_pat >= beta) return beta;
		if &#40;alpha < stand_pat&#41; alpha = stand_pat;
	&#125;
	//score & sort
	ScoreMoves&#40;);
	sort&#40;);
	for &#40;size_t i = 0; i < MoveCount; ++i&#41;
	&#123;
		//don't prune promotions ? check stock fish source code
		if ((!InCheck&#41;& (!PROMOTION&#41; && &#40;Position&#58;&#58;see_sign&#40;Available&#91;i&#93;) < 0&#41;) continue;

		QNodeCount++;

		Position&#58;&#58;MakeMove&#40;Available&#91;i&#93;);
		int Score = -q_search&#40;-beta, -alpha&#41;;
		Position&#58;&#58;UnMakeMove&#40;Available&#91;i&#93;);
		if &#40;Score >= beta&#41; return beta;
		if &#40;Score > alpha&#41; alpha = Score;
	&#125;
	if &#40;MoveCount == 0&#41; if &#40;InCheck&#41; return -Value_Mate + ply;
	return alpha;
&#125;


static int AlphaBeta&#40;int alpha, int beta, int depthleft, bool AllowNull&#41;
&#123;
	NNodeCount++;

	//insufficient material detection , or &#40;maxPly-1&#41; to avoid problems on extensions
	if &#40;IsDraw&#40;)) return Value_Draw;

	//Three-fold repetition detection
	int IndexofLastIrreversible = GameRecordCounter < HalfMoveClock ? 0 &#58; GameRecordCounter - HalfMoveClock;
	for &#40;int i = GameRecordCounter - 2; i >= IndexofLastIrreversible; i--) if &#40;Positionkey == GameRecord&#91;i&#93;) return Value_Draw;

	if &#40;depthleft == 0&#41; return q_search&#40;alpha, beta&#41;;
	if &#40;ply > maxPly&#41; maxPly = ply;


	TTEntry entry;
	bool found = false;
	//see &#58;http&#58;//www.stmintz.com/ccc/index.php?id=93732
	if (&#40;ply>1&#41; && &#40;found = TTProbe&#40;entry&#41;) && &#40;entry.Depth >= depthleft&#41;)
	&#123;
		if (&#40;entry.Score >= beta&#41; & &#40;entry.bound == BoundType&#58;&#58;Lower&#41;)
		&#123;
			if &#40;Quiet&#41; UpdateKillersAndHistory&#40;entry.move, depthleft&#41;;
			return beta;
		&#125;
		if (&#40;entry.Score <= alpha&#41; & &#40;entry.bound == BoundType&#58;&#58;Upper&#41;) return alpha;
		if &#40;entry.bound == BoundType&#58;&#58;Exact&#41;return entry.move.Score;
	&#125;


#if NullMovePruning
	int R = 2;
	if &#40;AllowNull &  (!InCheck&#41; &&#40;depthleft > &#40;R + 1&#41;) & SideToMoveNonPawnMaterial&#40;))//is it okay to descend directly to the quiescence search ?!
	&#123;
		R += &#40;depthleft > 7&#41;;//todo &#58; check if side to move has 2 or more pieces &#58;http&#58;//chessprogramming.wikispaces.com/Null+Move+Pruning+Test+Results
		//Make null move
		ply++;//not needed !		  
		STM = Color&#40;7 - STM&#41;;//reverse side to move
		U64 OldKey = Positionkey;
		Positionkey ^= ZobristSide;
		if &#40;DoublePushSQ&#91;1&#93; != 0&#41; Positionkey ^= GetEnpassantZobrist&#40;);
		unsigned int EpTOld = DoublePushSQ&#91;1&#93;;
		DoublePushSQ&#91;1&#93; = 0;
		//auto oldHMClock = HalfMoveClock;//Is a null move considered irreversible ?
		//HalfMoveClock = 0;
		int NMScore = -AlphaBeta&#40;-beta, -beta + 1, depthleft - 1 - R, false&#41;;
		//Unmake null move
		ply--;
		STM = Color&#40;7 - STM&#41;;
		DoublePushSQ&#91;1&#93; = EpTOld;
		//HalfMoveClock = oldHMClock;
		Positionkey = OldKey;
		if &#40;NMScore >= beta&#41; return beta;
	&#125;
#endif   
	//Used for Scoring & sorting
	ttMove = &#40;found ? entry.move &#58; Move&#40;));

	//simple Check Extension
	if &#40;InCheck&#41; depthleft++;//todo &#58;test making it per move depending on it's SEE value ?

	//score&sort &#58; todo replace with a selection sort and split the generator
	ScoreMoves&#40;);
	sort&#40;);

	Move mBest = Move&#40;);
	BoundType bound = BoundType&#58;&#58;Upper;

	for &#40;size_t i = 0; i < NMoves; i++)
	&#123;
		Position&#58;&#58;MakeMove&#40;Available&#91;i&#93;);
		auto Score =-AlphaBeta&#40;-beta, -alpha, depthleft - 1, true&#41;;
		Position&#58;&#58;UnMakeMove&#40;Available&#91;i&#93;);
		if &#40;Score >= beta&#41;
		&#123;
			if &#40;Quiet&#41; UpdateKillersAndHistory&#40;Available&#91;i&#93;, depthleft&#41;;
			TTStore&#40;Available&#91;i&#93;, depthleft, BoundType&#58;&#58;Lower, Score&#41;;
			return beta;
		&#125;
		if &#40;Score > alpha&#41;
		&#123;
			bound = BoundType&#58;&#58;Exact;
			mBest = Available&#91;i&#93;;
			alpha = Score;
		&#125;
	&#125;
	if &#40;NMoves == 0&#41;
	&#123;
		auto Score = InCheck ? -Value_Mate + ply &#58; Value_Draw;
		TTStore&#40;Move&#40;), depthleft, BoundType&#58;&#58;Exact, Score&#41;;
		return Score;
	&#125;

	TTStore&#40;mBest, depthleft, bound, alpha&#41;;
	return alpha;
&#125;



///isn't it wrong to descend  directly to qsearch at the first loop &#58; d-1=0
Move IterativeDeepning&#40;size_t maxdepth&#41;
&#123;
	Move BestMove;
	Move Available&#91;218&#93;;
	std&#58;&#58;pair<Move, int> RMoves&#91;218&#93;;
	for &#40;size_t i = 0; i < NMoves; i++) RMoves&#91;i&#93; = std&#58;&#58;make_pair&#40;Available&#91;i&#93;,Position&#58;&#58;SEE&#40;Available&#91;i&#93;));
	std&#58;&#58;stable_sort&#40;RMoves, RMoves + NMoves, SrtfunR&#41;;
	maxPly = 0;
	for &#40;size_t d = 1; &#40;d <= maxdepth&#41;; d++)
	&#123;
		int alpha = -Value_Mate, beta = Value_Mate;
		U64 PrevNodeCount = NNodeCount;
		NNodeCount = 0ULL;
		QNodeCount = 0ULL;
		U64 sw = 0;
		for &#40;size_t i = 0; i < NMoves; i++)
		&#123;
			Position&#58;&#58;MakeMove&#40;RMoves&#91;i&#93;.first&#41;;
			///******************************************************** d-1?
			U64 pnodes = NNodeCount;
			auto Score = -AlphaBeta&#40;-beta, -alpha, d - 1, true&#41;;
			RMoves&#91;i&#93;.second = INT32_MAX - &#40;NNodeCount - pnodes&#41;;
			Position&#58;&#58;UnMakeMove&#40;RMoves&#91;i&#93;.first&#41;;
			if &#40;Score > alpha&#41;
			&#123;
				alpha = Score;
				BestMove = RMoves&#91;i&#93;.first;
				for &#40;int j = sw - 1; j >= 0; j--) if &#40;j + 1 < NMoves&#41; std&#58;&#58;swap&#40;RMoves&#91;j&#93;, RMoves&#91;j + 1&#93;);
				std&#58;&#58;swap&#40;RMoves&#91;i&#93;, RMoves&#91;0&#93;);
				sw++;
			&#125;
		&#125;
		if &#40;abs&#40;alpha&#41; >= Value_Mate_In_Max_Ply&#41;//optimize this section
		&#123;
			std&#58;&#58;cout << FormatResult<MateResult>&#40;d, PrevNodeCount, TimeManager&#58;&#58;Elapsed&#40;), alpha, BestMove&#41; << std&#58;&#58;endl;
			break;
		&#125;
		else std&#58;&#58;cout << FormatResult<NormalResult>&#40;d, PrevNodeCount, TimeManager&#58;&#58;Elapsed&#40;), alpha, BestMove&#41; << std&#58;&#58;endl;
		if &#40;TimeManager&#58;&#58;Stop&#40;)) break;
		std&#58;&#58;stable_sort&#40;RMoves + 1, RMoves + NMoves, SrtfunR&#41;;
	&#125;
	return BestMove;
&#125;
1-Am I doing something wrong ?
2-is it okay to descend directly to qsearch after a null move ?
3-Can I use null move Reductions in endgame positions where null move pruning isn't applicable ?
5-Currently I'm using a basic TT with an Always Replace strategy , should I see a clear performance gain if I switch to a bucket system ?
4-What is the next step ? LMR ?
User avatar
phhnguyen
Posts: 1437
Joined: Wed Apr 21, 2010 4:58 am
Location: Australia
Full name: Nguyen Hong Pham

Re: Code review , and what is the next step ?

Post by phhnguyen »

Quick glance, your bellowing code looks dangerous IMO:

Code: Select all

   if (!InCheck&#41; 
It may be a bug if it is a global variable.

I guess your function ScoreMoves() is used to generate all legal moves. IMO it is so expensive for using in Quiescent Search.
Sven
Posts: 4052
Joined: Thu May 15, 2008 9:57 pm
Location: Berlin, Germany
Full name: Sven Schüle

Re: Code review , and what is the next step ?

Post by Sven »

MahmoudUthman wrote:My search routine :
[...]
1-Am I doing something wrong ?
2-is it okay to descend directly to qsearch after a null move ?
3-Can I use null move Reductions in endgame positions where null move pruning isn't applicable ?
5-Currently I'm using a basic TT with an Always Replace strategy , should I see a clear performance gain if I switch to a bucket system ?
4-What is the next step ? LMR ?
1) Hard to tell, it's a lot of code. See also my comment #4.

2) There are different opinions about it. I would say it may be risky. But your current nullmove code does not look like descending directly into qsearch since "depthleft - 1 - R" is always >= 1 under your precondition "depthleft > R + 1".

3) No. "Null move reduction" and "null move pruning" are two different terms for the same single concept. It is "reduction" since you search the null move to a reduced depth, and it is also "pruning" since on a fail-high of the null move you prune the whole subtree of the current node that would have been searched without trying the null move.

5) Usually yes, in terms of time to depth, since you should get significantly more TT hits.

4) That depends a lot on the quality of your previous steps. If you have already established a working test procedure to ensure that all your changes are working as expected and result in the appropriate improvement of playing strength then proceed with any (search) feature that you think fits best for you. However, if you haven't then I suggest not to proceed with new features, and build your testing system first. It is "quite easy" to add 8 features to your engine that improve strength by +25 Elo points each, and 2 that are buggy and cost you -100 points each, resulting in a +/- 0 ...

Some more points:

- The null move is usually considered as irreversible, although there are different opinions about it as well.

- You should increment "ply" also when making the null move, otherwise you could get unexpected results anywhere in the subtree where you use "ply" for something. Keep it simple, don't create too many exceptional cases.

- Use "&&" instead of "&" for logical AND. Otherwise you may get surprising results at some point in time.

- I would consider moving the check extension to the top of AlphaBeta(). That would avoid starting qsearch when being in check. You can still have a capture that gives check. But then please note that you have to ensure that your move generator produces *all* legal check evasions even in qsearch, since otherwise you would return a lot of wrong mate scores in qsearch.

- A question: it seems that your move generator is part of ScoreMoves(), is that true? In that case I would consider renaming it. Also I do not see how ScoreMoves() should know whether it should generate all moves or only those required for qsearch.
elpapa
Posts: 211
Joined: Sun Jan 18, 2009 11:27 pm
Location: Sweden
Full name: Patrik Karlsson

Re: Code review , and what is the next step ?

Post by elpapa »

Code: Select all

///isn't it wrong to descend  directly to qsearch at the first loop &#58; d-1=0 
Yes, it is. Well, maybe not technically wrong, but you're constantly searching 1 ply less than intended.

Code: Select all

auto Score = -AlphaBeta&#40;-beta, -alpha, d - 1, true&#41;; 
Here's the actual line. It's like you want to search to a certain depth and then change you're mind in the last second.
AlvaroBegue
Posts: 931
Joined: Tue Mar 09, 2010 3:46 pm
Location: New York
Full name: Álvaro Begué (RuyDos)

Re: Code review , and what is the next step ?

Post by AlvaroBegue »

elpapa wrote:

Code: Select all

///isn't it wrong to descend  directly to qsearch at the first loop &#58; d-1=0 
Yes, it is. Well, maybe not technically wrong, but you're constantly searching 1 ply less than intended.

Code: Select all

auto Score = -AlphaBeta&#40;-beta, -alpha, d - 1, true&#41;; 
Here's the actual line. It's like you want to search to a certain depth and then change you're mind in the last second.
I exactly disagree: Using d-1 is perfectly fine. d is the depth to which you wish to search the root of the tree, which means its children should be searched to depth d-1. A depth-1 search of the root will have you calling quiescence search for each child.
elpapa
Posts: 211
Joined: Sun Jan 18, 2009 11:27 pm
Location: Sweden
Full name: Patrik Karlsson

Re: Code review , and what is the next step ?

Post by elpapa »

AlvaroBegue wrote:
elpapa wrote:

Code: Select all

///isn't it wrong to descend  directly to qsearch at the first loop &#58; d-1=0 
Yes, it is. Well, maybe not technically wrong, but you're constantly searching 1 ply less than intended.

Code: Select all

auto Score = -AlphaBeta&#40;-beta, -alpha, d - 1, true&#41;; 
Here's the actual line. It's like you want to search to a certain depth and then change you're mind in the last second.
I exactly disagree: Using d-1 is perfectly fine. d is the depth to which you wish to search the root of the tree, which means its children should be searched to depth d-1. A depth-1 search of the root will have you calling quiescence search for each child.
I don't think it would. This is what his search function looks like, which I believe is the normal way of doing it:

Code: Select all

int alphabeta&#40;...)
&#123;

  if &#40;depthleft == 0&#41; return q_search&#40;alpha, beta&#41;;
  
  ...
  
  for&#40;each move&#41; &#123;
    ...
  &#125;
  ...
&#125;
If you call it with depth 0, it will just return the q_search value for the root, without making any moves.
AlvaroBegue
Posts: 931
Joined: Tue Mar 09, 2010 3:46 pm
Location: New York
Full name: Álvaro Begué (RuyDos)

Re: Code review , and what is the next step ?

Post by AlvaroBegue »

elpapa wrote:
AlvaroBegue wrote:
elpapa wrote:

Code: Select all

///isn't it wrong to descend  directly to qsearch at the first loop &#58; d-1=0 
Yes, it is. Well, maybe not technically wrong, but you're constantly searching 1 ply less than intended.

Code: Select all

auto Score = -AlphaBeta&#40;-beta, -alpha, d - 1, true&#41;; 
Here's the actual line. It's like you want to search to a certain depth and then change you're mind in the last second.
I exactly disagree: Using d-1 is perfectly fine. d is the depth to which you wish to search the root of the tree, which means its children should be searched to depth d-1. A depth-1 search of the root will have you calling quiescence search for each child.
I don't think it would. This is what his search function looks like, which I believe is the normal way of doing it:

Code: Select all

int alphabeta&#40;...)
&#123;

  if &#40;depthleft == 0&#41; return q_search&#40;alpha, beta&#41;;
  
  ...
  
  for&#40;each move&#41; &#123;
    ...
  &#125;
  ...
&#125;
If you call it with depth 0, it will just return the q_search value for the root, without making any moves.
But if you look at how he does iterative deepening, the "d-1" call is inside a loop over the moves. So we are not talking about q_search of the root:

Code: Select all

    for &#40;size_t i = 0; i < NMoves; i++) 
      &#123; 
         Position&#58;&#58;MakeMove&#40;RMoves&#91;i&#93;.first&#41;; 
         //...
         auto Score = -AlphaBeta&#40;-beta, -alpha, d - 1, true&#41;;
elpapa
Posts: 211
Joined: Sun Jan 18, 2009 11:27 pm
Location: Sweden
Full name: Patrik Karlsson

Re: Code review , and what is the next step ?

Post by elpapa »

AlvaroBegue wrote:But if you look at how he does iterative deepening, the "d-1" call is inside a loop over the moves. So we are not talking about q_search of the root:
Yes, you're right. I missed that outer move loop. Probably because I don't to it that way myself.

Edit: Too much code to go through, and maybe there's a reason for it, but removing that loop and just call alphabeta with d instead of d-1 seems like cleaner solution, though.
MahmoudUthman
Posts: 234
Joined: Sat Jan 17, 2015 11:54 pm

Re: Code review , and what is the next step ?

Post by MahmoudUthman »

phhnguyen wrote:Quick glance, your bellowing code looks dangerous IMO:

Code: Select all

   if (!InCheck&#41; 
It may be a bug if it is a global variable.

I guess your function ScoreMoves() is used to generate all legal moves. IMO it is so expensive for using in Quiescent Search.
It's a local variable , I removed the declaration and the some parts of the code like calls to move generation , sort arguments ,... etc. to shorten the code.
Sven Schüle wrote:
MahmoudUthman wrote:My search routine :
[...]
1-Am I doing something wrong ?
2-is it okay to descend directly to qsearch after a null move ?
3-Can I use null move Reductions in endgame positions where null move pruning isn't applicable ?
5-Currently I'm using a basic TT with an Always Replace strategy , should I see a clear performance gain if I switch to a bucket system ?
4-What is the next step ? LMR ?
3) No. "Null move reduction" and "null move pruning" are two different terms for the same single concept. It is "reduction" since you search the null move to a reduced depth, and it is also "pruning" since on a fail-high of the null move you prune the whole subtree of the current node that would have been searched without trying the null move.
3- I meant something like this :

Code: Select all

bool LateEndgame=NonPawnMaterial==0;
if&#40;AllowNull&#41;
makeNullMove
score=alphabeta ....
unmakeNullmove
if&#40;Score>beta&#41;
&#123;
if&#40;LateEndgame&#41;
&#123;
 depth -= 4; // reduce search
      if ( depth <= 0 )
          return Quiescence&#40;);
&#125;
else return beta;//prune search
&#125;
- A question: it seems that your move generator is part of ScoreMoves(), is that true? In that case I would consider renaming it. Also I do not see how ScoreMoves() should know whether it should generate all moves or only those required for qsearch.
No, I just removed it to shorten the code
Sven
Posts: 4052
Joined: Thu May 15, 2008 9:57 pm
Location: Berlin, Germany
Full name: Sven Schüle

Re: Code review , and what is the next step ?

Post by Sven »

MahmoudUthman wrote:
Sven Schüle wrote:
MahmoudUthman wrote:3-Can I use null move Reductions in endgame positions where null move pruning isn't applicable ?
3) No. "Null move reduction" and "null move pruning" are two different terms for the same single concept. It is "reduction" since you search the null move to a reduced depth, and it is also "pruning" since on a fail-high of the null move you prune the whole subtree of the current node that would have been searched without trying the null move.
3- I meant something like this :

Code: Select all

bool LateEndgame=NonPawnMaterial==0;
if&#40;AllowNull&#41;
makeNullMove
score=alphabeta ....
unmakeNullmove
if&#40;Score>beta&#41;
&#123;
if&#40;LateEndgame&#41;
&#123;
 depth -= 4; // reduce search
      if ( depth <= 0 )
          return Quiescence&#40;);
&#125;
else return beta;//prune search
&#125;
In positions where the moving side has no non-pawn material the null move is not applicable due to a high risk of overlooking zugzwang. But you don't detect zugzwang by performing a quiescence search. Therefore I'd say your idea would probably not work.