Simplifying code

[Henk]

O wait I forgot a !. That explains the performance difference.


So code I wrote yersterday was not correct.

stage == 3 && allowStandPat && !p.position.GivesCheck(mv)

[Sven]

In your last code snippet the second condition "(i >= prioQ.Count())" is always false due to the previous one ...

What was two times slower, the whole search?

[Henk]

Yes whole search is two times slower to something that was incorrect. So comparison does not make sense.

Pity I thought speed was back to 600-1000kn/second. But It is hardly 300kn/second.

By the way making ChessPosition a struct did not work too for linq was complaining about that properties were not accessable
for a struct using that linq construction.

Only works if I make method static and pass position as an argument. But that means it copies a position. So not efficient.
By the way I have more methods with position as an argument. So that would mean many unnecessary copies.

So ChessPosition stays a class so passing it will be by reference.

I made ChessBoardBits a struct. But no performance gain as expected. But I did not check any further. Might be there are methods with ChessBoardBits as a parameter giving unnecessary copies slowing down performance.

Code: Select all

 public void CollectCaptures(Builder builder, ulong source, ulong target,
            bool excludePromotions)
        {
            var iter = new BitBoardIterator(0);
            builder.AddRange(
                new BitBoardIterator(source).Select(b =>
                 {
                     var sq = BoardSquares[b];
                     var sort = Board.PieceSort(sq);
                     iter.Init(sq.Moves(sort, Board.Occupiers) & target);
                     return CollectCaptures(iter, sq.MovesDict[(int)sort], excludePromotions);
                 }
               ).Aggregate((x, y) => x.AddRange(y))
           );
        }
 

Code above does not compile if ChessPosition is a struct.

Using your code for NextMove

Code: Select all

  (IMovePrioQ, IMoveBase, int, int) NextMove(SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
        {
            if (stage > 3)
            {
                return (prioQ, null, -1, -1);
            }

            if (i >= prioQ.Count())
            {
                if (stage == 3) return (prioQ, null, -1, -1);
                return NextMove(p, CollectPseudoLegalMoves(p, stage + 1, allowStandPat), stage + 1, 0, allowStandPat);
            }
            var mv = prioQ[i];     
            if (stage > 1 && !LegalMove(p.inCheck, mv, p.position)
                || stage == 3 && allowStandPat && !p.position.GivesCheck(mv))
            {
                    return NextMove(p, prioQ, stage, i + 1, allowStandPat);
            }
            else
                return (prioQ, mv, i, stage);
        }
        

Only difference i before stage in return value.

[Sven]

O wait I forgot a !. That explains the performance difference.


So code I wrote yersterday was not correct.

stage == 3 && allowStandPat && !p.position.GivesCheck(mv)

Which ! was missing? The one in this snippet above has always been there since you posted your NextMove() code. Omitting it would mean (if I understand your code correctly) that your qsearch would include all non-checking quiet moves if allowStandPat == true, which would effectively search one ply deeper than desired (but less accurate due to omitting quiet checks).

[Henk]

It is about this code. Don't ask me what it is doing. I think it is crap. At least I don't understand it.

Code: Select all

           bool? noMoreMovesInStandPat = null;
            if (!noMove)
            {
                noMoreMovesInStandPat = stage == 3 && allowStandPat && p.position.GivesCheck(mv);
                noMove = noMoreMovesInStandPat.Value;
            }

            if (noMove)
            {
                if (noMoreMovesInStandPat.HasValue && noMoreMovesInStandPat.Value)
                    return (prioQ, null, -1, -1);
                else
                    return NextMove(p, prioQ, stage, i + 1, allowStandPat);
            }
            else
                return (prioQ, mv, i, stage);
  

By the way I also restore ChessBoardBits. Making it a class again. For I like to control when object is copied and in case of struct it is always a copy. Or I should use ''ref' in C#. But the compiler complains that ref can not be used for a property and ChessBoardBits is a property of ChessPosition.

[Sven]

I don't ask ... but I agree that it looks suspicious. The meaning of "noMoreMovesInStandPat" is unclear and its implementation looks wrong since it appears to cut off all possibly remaining quiet checks during qsearch based on a property of just the current move (which is a check or not, depending on whether you forget the "!" before "p.position.GivesCheck(mv)" or not). So regardless whether the "!" is present, I think the whole version of NextMove() that uses this "noMoreMovesInStandPat" variable was useless, so it seems your decision to revert it should be fine.

Now if you compare search speed with NextMove() in this "old" version which you initially posted:

Code: Select all

    (IMovePrioQ, IMoveBase, int, int) NextMove( SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
        {         
            var end = i >= prioQ.Count();
            var (j, nextStage) = end ? (0, stage+1) : (i, stage);
            var nextPrioQ = (end && (nextStage == 2 || nextStage == 3)) ? new MovePrioQ(  
                       (nextStage == 2 ? p.position.CollectCapturesPromotions(true): p.position.CollectNonCaptures()
                       ).Select(m =>
                         (ComputePriority(PSQTableEval, allowStandPat, KillerMoves,
                          MoveRatings, m, p.position, p.depth), m)).ToImmutableList()).Sort()
                 : prioQ;
            var mv = j < nextPrioQ.Count() ? nextPrioQ[j]: null;
            return  (mv == null 
                || nextStage > 1 &&   !LegalMove(p.inCheck, mv, p.position)
                || nextStage == 3 && allowStandPat && !p.position.GivesCheck(mv)
                || nextStage > 3
                    )  
            ? (nextStage > 3) ? (nextPrioQ, null, -1, -1): NextMove(p, nextPrioQ, nextStage, j + 1, allowStandPat)        
            : (nextPrioQ, mv, j, nextStage);
        }

to your current version:

Code: Select all

  (IMovePrioQ, IMoveBase, int, int) NextMove(SearchParam p, IMovePrioQ prioQ, int stage, int i, bool allowStandPat)
        {
            if (stage > 3)
            {
                return (prioQ, null, -1, -1);
            }

            if (i >= prioQ.Count())
            {
                if (stage == 3) return (prioQ, null, -1, -1);
                return NextMove(p, CollectPseudoLegalMoves(p, stage + 1, allowStandPat), stage + 1, 0, allowStandPat);
            }
            var mv = prioQ[i];     
            if (stage > 1 && !LegalMove(p.inCheck, mv, p.position)
                || stage == 3 && allowStandPat && !p.position.GivesCheck(mv))
            {
                    return NextMove(p, prioQ, stage, i + 1, allowStandPat);
            }
            else
                return (prioQ, mv, i, stage);
        }

Is there any substantial difference in overall search speed? And do both work correctly (or at least, functionally identical)?

[Henk]

I have to test later. System not alive. For in the meantime I started rewriting ChessBoardBits. But is not working yet. Being obsessed by using immutable datatypes. So I started to make array of bitboards immutable.

Think both versions posted are equally fast/slow because generating pseudo legal moves is slowest operation. Second place is test whether move is legal.

Current version of eval is doing not much more than returning piece square value.

[Also have a plan to add an extra stage for collecting captures of heavy pieces.
For if a pawn captures a rook there is a good chance no extra moves need to be generated]

[Henk]

Ok results from initial position. First one is yours. Second one was my original one.
So about same speed for speed of my pc is not constant.

Code: Select all

  
   1      27            6              2    b1c3
   2      19            8             45    b1c3  b8c6
   3      27            9            213    b1c3  b8c6  g1f3
   4      19           11           2186    b1c3  b8c6  g1f3  g8f6
   5     -20           14           5220    b1c3  b8c6  g1f3  g8f6  a1b1
   6      19           27          17299    b1c3  b8c6  g1f3  g8f6  a1b1  a7a5
   7      19           53          42266    b1c3  b8c6  g1f3  g8f6  e2e4  a7a5  f1d3
   8      19          130         131742    b1c3  b8c6  g1f3  g8f6  e2e4  e7e5  f1d3  f8d6
   9      15          431         531100    b1c3  b8c6  d2d4  g8f6  c1f4  f6h5  f4e3  h5f6  g1f3
  10      15         1275        1581668    b1c3  b8c6  d2d4  g8f6  c1f4  e7e6  g1f3  f8e7  a1b1  e8g8
  11      13         2993        4210710    b1c3  b8c6  d2d4  g8f6  d4d5  c6e5  g1f3  e5f3  e2f3  e7e6  d5e6  f7e6

   1      27            7              2    b1c3
   2      19           10             45    b1c3  b8c6
   3      27           11            213    b1c3  b8c6  g1f3
   4      19           14           2186    b1c3  b8c6  g1f3  g8f6
   5     -20           18           5220    b1c3  b8c6  g1f3  g8f6  a1b1
   6      19           33          17299    b1c3  b8c6  g1f3  g8f6  a1b1  a7a5
   7      19           60          42266    b1c3  b8c6  g1f3  g8f6  e2e4  a7a5  f1d3
   8      19          132         131742    b1c3  b8c6  g1f3  g8f6  e2e4  e7e5  f1d3  f8d6
   9      15          434         531100    b1c3  b8c6  d2d4  g8f6  c1f4  f6h5  f4e3  h5f6  g1f3
  10      15         1292        1581668    b1c3  b8c6  d2d4  g8f6  c1f4  e7e6  g1f3  f8e7  a1b1  e8g8
  11      13         3138        4210710    b1c3  b8c6  d2d4  g8f6  d4d5  c6e5  g1f3  e5f3  e2f3  e7e6  d5e6  f7e6
  

[Henk]

Don't know. I get the impression that if you adopt functional programming style [I mean writing code that doesn't update variables] then you have to create a function for almost each loop or if statement.

Otherwise you may end up writing complicated nested expressions that are difficult to maintain or worse no one understands.

[Henk]

First I had this. But I don't want to create a class for each method that updates the state (say TranspositionTable, NodeCount, KillerMoves etc)

Code: Select all

   public interface IStateFunc<B, S>
    {
        (B res, S state) Apply();
    }

  class ABState<S>
    {
        public S State
        {
            get;
            private set;
        }

        public ABState(S state)
        {
            State = state;
        }
        public B Update<B>(IStateFunc<B, S> f)
        {
            var (res, state) = f.Apply();
            State = state;
            return res;
        }
    }
    

So second try:

Code: Select all

 class ABState2<S>
    {
        public S State
        {
            get;
            private set;
        }

        public ABState2(S state)
        {
            State = state;
        }
        public B Update<A1, A2, A3, A4, A5, A6, A7, A8,B>
            (Func<A1, A2, A3, A4, A5, A6, A7, A8, (B, S) > f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5, A6 a6, A7 a7, A8 a8)
        {
            var (res, state) = f(a1, a2, a3, a4, a5, a6, a7, a8);
            State = state;
            return res;
        }
    }

Because function below has eight parameters.

Code: Select all

     Func<ABSearch, int, int, IMoveBase, SearchParam,bool, double, StateContent, ((int?, ILine), StateContent)> CompMvValue
            = delegate
        (ABSearch search, int mvCount, int lb, IMoveBase mv,SearchParam p, bool check, double nextPsqValue, StateContent stateContent)
        {
            var nextPosHistory = search.UpdateHistory(p, p.positionHistory);
            var nextPos = p.position.Move(mv);

            if (p.depth <= 0
                 || mvCount <= 0
                 || (p.inCheck || check)
                 || mv.GetCaptureLocation(p.position) != null

                 )
            {
                var (result, nextNodeCount, nextTTable) = search.DoABSearch(new SearchParam(stateContent.NodeCount + 1, false, nextPsqValue, nextPos,
                                  nextPosHistory, null,
                                  p.depth - R, p.plyCount + 1, -p.ub, -lb, check), stateContent.TTable);
                return !result.value.HasValue ? ((null, null), new StateContent(nextNodeCount, nextTTable))
                   : ((-result.value, result.bestLine), 
                   new StateContent(nextNodeCount, nextTTable));

            }
            else
            {

                int reduction = Min(p.depth, p.depth > 2 * R ? R + mvCount * p.depth / 50 : R);
                var (result, nextNodeCount, nextTTable) = search.DoABSearch(new SearchParam(stateContent.NodeCount + 1, false, nextPsqValue, nextPos, nextPosHistory, null,
                             p.depth - reduction, p.plyCount + 1, -(lb + 1), -lb, check), stateContent.TTable);
                if (!result.value.HasValue)
                {
                    return ((null, null), new StateContent(nextNodeCount, nextTTable));
                }
                var mvVal = -result.value;

                if (mvVal > lb && (lb != p.ub - 1 || reduction > R))
                {
                    var (result2, nextNodeCount2, nextTTable2) = search.DoABSearch(new SearchParam(nextNodeCount, false, nextPsqValue, nextPos, nextPosHistory, null,
                             p.depth - R, p.plyCount + 1, -p.ub, -lb, check), nextTTable);
                    var researchValue = !result2.value.HasValue ? null : -result2.value;
                    return ((researchValue, result2.bestLine), new StateContent(nextNodeCount2, nextTTable2));
                }
                else
                {
                    return ((mvVal, result.bestLine), new StateContent(nextNodeCount, nextTTable));
                }
            }
        };

But then you get a call like this.

Code: Select all

     ...
      var abState2 = new ABState2<StateContent>(stateContent);
       var (mvValue, counterLine) = abState2.Update
                <ABSearch, int, int, IMoveBase, SearchParam,bool, double,
                  StateContent, (int?, ILine)> 
                  (CompMvValue, this, mvCount, lb, mv, p, check, nextPsqValue, stateContent);
   ...
 

Really simplfying (not).