Merge pull request opencog#1450 from linas/max-cost-fix

Fix very old max cost bug

ChangeLog

@@ -9,6 +9,7 @@ Version 5.12.1 (XXX 2023)
  * English dict: paraphrasing fixes. #1398
  * Report CPU time usage only for the current thread. #1399
  * Extensive performance optimizations for MST dictionaries. #1402
+ * Fix incorrect maxcost computation. This is a very old bug. #1450
 
 Version 5.12.0 (26 Nov 2022)
  * Fix crash when using the Atomese dictionary backend.

link-grammar/prepare/build-disjuncts.c

@@ -33,8 +33,8 @@ typedef struct clause_struct Clause;
 struct clause_struct
 {
 	Clause * next;
-	float cost;
-	float maxcost;
+	float totcost;   // Total cost of all connectors in the clause
+	float maxcost;   // Cost of the most costly lingle connector.
 	Tconnector * c;
 };
 
@@ -138,7 +138,7 @@ static Clause * build_clause(Exp *e, clause_context *ct, Clause **c_last)
 		Clause *c1 = pool_alloc(ct->Clause_pool);
 		c1->c = NULL;
 		c1->next = NULL;
-		c1->cost = 0.0;
+		c1->totcost = 0.0;
 		c1->maxcost = 0.0;
 		for (Exp *opd = e->operand_first; opd != NULL; opd = opd->operand_next)
 		{
@@ -148,14 +148,10 @@ static Clause * build_clause(Exp *e, clause_context *ct, Clause **c_last)
 			{
 				for (Clause *c4 = c2; c4 != NULL; c4 = c4->next)
 				{
-					float maxcost = MAX(c3->maxcost,c4->maxcost);
-					/* Cannot use this shortcut due to negative costs. */
-					//if (maxcost + e->cost > ct->cost_cutoff) continue;
-
 					Clause *c5 = pool_alloc(ct->Clause_pool);
 					if ((c_head == NULL) && (c_last != NULL)) *c_last = c5;
-					c5->cost = c3->cost + c4->cost;
-					c5->maxcost = maxcost;
+					c5->maxcost = MAX(c3->maxcost, c4->maxcost);
+					c5->totcost = c3->totcost + c4->totcost;
 					c5->c = catenate(c4->c, c3->c, ct->Tconnector_pool);
 					c5->next = c_head;
 					c_head = c5;
@@ -191,7 +187,7 @@ static Clause * build_clause(Exp *e, clause_context *ct, Clause **c_last)
 	{
 		c = pool_alloc(ct->Clause_pool);
 		c->c = build_terminal(e, ct);
-		c->cost = 0.0;
+		c->totcost = 0.0;
 		c->maxcost = 0.0;
 		c->next = NULL;
 		if (c_last != NULL) *c_last = c;
@@ -204,14 +200,19 @@ static Clause * build_clause(Exp *e, clause_context *ct, Clause **c_last)
 	/* c now points to the list of clauses */
 	for (Clause *c1 = c; c1 != NULL; c1 = c1->next)
 	{
-		c1->cost += e->cost;
-		/* c1->maxcost = MAX(c1->maxcost,e->cost);  */
-		/* Above is how Dennis had it. Someone changed it to below.
-		 * However, this can sometimes lead to a maxcost that is less
-		 * than the cost ! -- which seems wrong to me ... seems Dennis
-		 * had it right!?
+		/* The maxcost is the most costly single connector in the
+		 * expression. It is used, in conjunction with the cost_cutoff,
+		 * to reject clauses which contain a single costly connector
+		 * in them. This allows cost_cutoff to be raised for panic
+		 * parses, thus allowing perhaps-dubious disjuncts into the
+		 * parse.  Note that maxcost can be less than totcost, because
+		 * the totcost might be the sum of many low-cost connectors,
+		 * sum sums can get large. (maxcost is the Banach l_0 norm,
+		 * while totcost is the Banach l_1 norm).
 		 */
-		c1->maxcost += e->cost;
+		c1->maxcost = MAX(c1->maxcost, e->cost);
+		c1->totcost += e->cost;
+
 		/* Note: The above computation is used as a saving shortcut in
 		 * the inner loop of AND_type. If it is changed here, it needs to be
 		 * changed there too. */
@@ -286,7 +287,7 @@ build_disjunct(Sentence sent, Clause * cl, const char * string,
 		if (sat_solver || (!IS_GENERATION(sent->dict) || (' ' != string[0])))
 		{
 			ndis->word_string = string;
-			ndis->cost = cl->cost;
+			ndis->cost = cl->totcost;
 			ndis->is_category = 0;
 		}
 		else
@@ -300,8 +301,8 @@ build_disjunct(Sentence sent, Clause * cl, const char * string,
 			assert(sat_solver || ((ndis->category[0].num > 0) &&
 			       (ndis->category[0].num < 64*1024)),
 			       "Insane category %u", ndis->category[0].num);
-			ndis->category[0].cost = cl->cost;
-			// ndis->cost = cl->cost; No! clobbers memory!
+			ndis->category[0].cost = cl->totcost;
+			// ndis->cost = cl->totcost; No! clobbers memory!
 		}
 
 		ndis->originating_gword = (gword_set*)gs; /* XXX remove constness */
@@ -364,7 +365,7 @@ GNUC_UNUSED static void print_clause_list(Clause * c)
 {
 	for (;c != NULL; c=c->next) {
 		printf("  Clause: ");
-		printf("(%4.2f, %4.2f) ", c->cost, c->maxcost);
+		printf("(%4.2f, %4.2f) ", c->totcost, c->maxcost);
 		print_Tconnector_list(c->c);
 		printf("\n");
 	}