Re: nando_3_9.4b (was: RE: [gnugo-devel] nando_3_9.4)

[Arend Bayer]

Portela Fernand wrote:

> - includes and supercedes nando_3_9.4 and nando_3_9.4a
> - dragon status is set to ALIVE in case of GAIN/LOSS attack/defense
>   codes
> - .what field for OWL_ATTACK_MOVE_GAIN and OWL_DEFEND_MOVE_LOSS is now
>   an index into the either_data array, which is reused for its .what1
>   and .what2 fields. These moves are similar to EITHER_MOVE, so this is
>   logical. And in case it's needed, it will be easy to create a separate
>   array.
> - added a couple supporting functions : add_gain_move(), add_loss_move()
>   and find_pair_data()
> - extended and renamed catalog_worm() to something else to prevent
>   confusion. The older code is kept as it was, i.e. disabled by a #if 0
>   directive.

Although I still have a couple of suggestions, I think we should put the
patch in CVS as is, to avoid having it outdated again (assuming
regressions still don't show any breakage). Of course it can be improved
further, but that will be easier once it is in CVS.

A few comments:
* Please use "acode" for attack codes and "dcode" for defend,
reversing this (in owl_confirm_safety) is really confusing...
(Also I tend to confuse LOSE and LOSS, so maybe we stick with 0 for the
former?)
* I think owl_attack returning GAIN with kworm == NO_MOVE should be
considered a bug. E.g. the move valuation shouldn't have to work around this.
I have tried to resolve this, but didn't get it completely consistent.
(I will be on travel for a week from tomorrow on, so I won't do more
work on this for a while -- can try to post a patch for what I've tried
so far if desired.)


And more comments on details:

> Index: engine/liberty.h
> ===================================================================
> @@ -798,6 +801,8 @@
>    int owl_defense_certain; /* 0 if owl reading node limit is reached         
> */
>    int owl_second_defense_point;/* if defender gets both attack points, wins  
> */
>    int status;              /* best trusted status                            
> */
> +  int owl_attack_kworm;    /* only valid when owl_attack_code is             
> */
> +  int owl_defense_kworm;   /* OWL_ATTACK_MOVE_GAIN or OWL_DEFEND_MOVE_LOSS   
> */
>  };
Should be GAIN or LOSS in the comment I suppose. And the latter for
owl_defense_code, or course.

> Index: engine/owl.c
> ===================================================================
> RCS file: /cvsroot/gnugo/gnugo/engine/owl.c,v
> retrieving revision 1.106
> diff -u -r1.106 owl.c
> --- engine/owl.c      25 Sep 2002 19:39:01 -0000      1.106
> +++ engine/owl.c      26 Sep 2002 09:24:16 -0000
>            close_pattern_list(other, &shape_patterns);
>         READ_RETURN(read_result, move, mpos, WIN);
>       }
> +#if 1
> +     /* the conditions here are set so that this code doesn't get
> +        triggered when the capture is immediate (the tactical reading
> +        code should take care of these) */
> +     else if (goal_worm_computed
> +#if 0
> +              && stackp>1
> +#endif
> +              && captured>2) {
> +       int w = MAX_GOAL_WORMS;
> +       int size = 0;
> +       int l;
> +       /* locate the biggest captured worm */
> +       for (l=0; l<MAX_GOAL_WORMS; l++) {
> +         if (owl_goal_worm[l] == NO_MOVE) break;
> +         if (board[owl_goal_worm[l]] == EMPTY)
> +           if (size == 0 || worm[owl_goal_worm[l]].size > size) {
> +             w = l;
> +             size = worm[owl_goal_worm[l]].size;
> +           }
> +       }
> +       if (w != MAX_GOAL_WORMS)
> +         if (LOSS > savecode) {
Shouldn't this be "GAIN > savecode"?
> +           /* if new result better, just update */
> +           dcode = LOSS;
> +           saveworm = w;
> +         }
> +         else if (LOSS == savecode) {
> +           /* bigger ? */
> +           int wpos = owl_goal_worm[saveworm];
> +           if (size > worm[wpos].size)
> +             saveworm = w;
> +         }
> +     }
> +     else if (dcode == LOSS)
> +       saveworm = wid;

I think this last "else if" should be moved before the "if (captured > 2"
etc. stuff, and should also make similar comparisons (check whether worm
"wid" is bigger than saveworm, in we case already captured s.th. in another
variation, etc.). E.g. if we have captured s.th., that is not in the
list, _and_ dcode is LOSS, then savecode will be updated to GAIN by
UPDATE_SAVED_KO_RESULT below, but saveworm is not set.


> -           dragon[pos].owl_second_attack_point, pos, movenum+1);
> +    else if (dragon[pos].owl_status == ALIVE) {
(...)
> +      else if (board[pos] == OTHER_COLOR(color)
> +            && dragon[pos].owl_attack_point != NO_MOVE
> +            && dragon[pos].owl_attack_code == GAIN
> +            && dragon[pos].owl_attack_kworm != NO_MOVE ) {
> +     add_gain_move(dragon[pos].owl_attack_point, pos,
> +                   dragon[pos].owl_attack_kworm);
> +     /* FIXME: add debug output */
> +      }
> +      else if (board[pos] == color
> +            && dragon[pos].owl_defense_point != NO_MOVE
> +            && dragon[pos].owl_defense_code == LOSS
> +            && dragon[pos].owl_defense_kworm != NO_MOVE ) {
> +     add_loss_move(dragon[pos].owl_defense_point, pos,
> +                   dragon[pos].owl_defense_kworm);
> +     /* FIXME: add debug output */
I think the second case is not necessarry. If the defense code is LOSS,
then attack code will probably have been "WIN", and the dragon is
critical. However, it seems to me you miss the case where the attack
code on our dragon is GAIN and defense code WIN. I think this should be
a standard OWL_DEFEND_MOVE, with a special case treatment in
estimate_territorial_value().

> +#if 0
> +      /* doesn't this overlap with the ' this_value *= 2.0' a couple
> +         lines above ? */
>        this_value = 2 * dragon[aa].effective_size;
> +#endif
Yes this should be removed.
> @@ -1574,6 +1626,11 @@
>       TRACE("  %1m: -%f - owl attack/defense of %1m only with bad ko\n",
>             pos, this_value, aa);
>        }
> +      else if (move_reasons[r].type == OWL_ATTACK_MOVE_GAIN
> +            || move_reasons[r].type == OWL_DEFEND_MOVE_LOSS) {
> +     /* FIXME: not too sure what to do here... */
> +     this_value = 0.0;
This looks right.
> +      }
>
>        tot_value -= this_value;
>

> @@ -1815,8 +1872,19 @@
>        case DEFEND_MOVE:
>        case DEFEND_MOVE_GOOD_KO:
>        case DEFEND_MOVE_BAD_KO:
> -     worm1 = move_reasons[r].what;
> -     aa = worms[worm1];
> +#if 0
> +      case OWL_ATTACK_MOVE_GAIN:
> +      case OWL_DEFEND_MOVE_LOSS:
> +#endif
> +     if (move_reasons[r].type == OWL_ATTACK_MOVE_GAIN
> +         || move_reasons[r].type == OWL_DEFEND_MOVE_LOSS) {
> +       worm1 = worms[either_data[move_reasons[r].what].what2];
> +       aa = worms[worm1];
> +     }
> +     else {
> +       worm1 = move_reasons[r].what;
> +       aa = worms[worm1];
> +     }
(This is in estimate_strategical_value).
At the moment, all OWL_DEFEND/ATTTACK moves don't have a strategic
valuation (this is worth discussing, of course). So I don't think
OWL_ATTACK_MOVE_GAIN should be there either.

Arend