Fix polyratfun bugs caused by incorrect clean flag

check/fixes.frm

@@ -2173,6 +2173,122 @@ print +s;
 assert succeeded?
 assert result("A") =~ expr("+N1")
 *--#] Issue325 :
+*--#[ Issue336_1 :
+#-
+CFunction rat,f,tag;
+Symbol a,b,y,x;
+
+* Build lots of combinations
+Local test = (
+	+ f(1)
+	+ f(1+x)
+	+ f(1+x+x^2)
+	+ f(x+x^2)
+	+ f(1+x^-1)
+	+ f(1+x^-2)
+	+ f(1+x+x^-1)
+	+ f(x+x^-1)
+	+ f(x^-1)
+	+ f(x^-2)
+	+ f(x^-1+x^-2)
+	+ f(1+x+y)
+	+ f(1+x+x^2+y+y^2)
+	+ f(x+x^2+y+y^2)
+	+ f(1+x^-1+y^-1)
+	+ f(1+x^-2+y^-2)
+	+ f(1+x+x^-1+y+y^-1)
+	+ f(x+x^-1+y+y^-1)
+	+ f(x^-1+y^-1)
+	+ f(x^-2+y^-2)
+	+ f(x^-1+x^-2+y^-1+y^-2)
+	+ f(x^-1*x^-1)
+	+ f(x^-2+x^-1*y^-1+y^-2)
+	+ f(x^2+x^1*y^-1+y^-2)
+	)^2;
+DropCoefficient;
+* Create rational polys, with both orderings of the num and den
+Identify f(a?)*f(b?) = rat(a,b) + rat(a,b);
+.sort
+DropCoefficient;
+#$i = 0;
+Multiply tag($i);
+$i = $i+1;
+Print +s;
+.sort
+
+* Everything should cancel in the end, and we should get zero.
+Identify rat(?a) = rat(?a) - x*rat(?a);
+Bracket x;
+.sort
+
+PolyRatFun rat;
+Collect f;
+.sort
+Identify x = 1;
+Identify f(x?) = x;
+
+Print;
+.end
+assert succeeded?
+assert result("test") =~ expr("0")
+*--#] Issue336_1 :
+*--#[ Issue336_2 :
+#-
+CFunction rat,f,tag;
+Symbol a,b,y,x;
+
+* Build lots of combinations
+Local test = (
+	+ f(1)
+	+ f(1+x)
+	+ f(1+x+x^2)
+	+ f(x+x^2)
+	+ f(1+x^-1)
+	+ f(1+x^-2)
+	+ f(1+x+x^-1)
+	+ f(x+x^-1)
+	+ f(x^-1)
+	+ f(x^-2)
+	+ f(x^-1+x^-2)
+	+ f(1+x+y)
+	+ f(1+x+x^2+y+y^2)
+	+ f(x+x^2+y+y^2)
+	+ f(1+x^-1+y^-1)
+	+ f(1+x^-2+y^-2)
+	+ f(1+x+x^-1+y+y^-1)
+	+ f(x+x^-1+y+y^-1)
+	+ f(x^-1+y^-1)
+	+ f(x^-2+y^-2)
+	+ f(x^-1+x^-2+y^-1+y^-2)
+	+ f(x^-1*x^-1)
+	+ f(x^-2+x^-1*y^-1+y^-2)
+	+ f(x^2+x^1*y^-1+y^-2)
+	)^2;
+DropCoefficient;
+* Create rational polys, with both orderings of the num and den
+Identify f(a?)*f(b?) = rat(a,b) + rat(a,b);
+.sort
+DropCoefficient;
+#$i = 0;
+Multiply tag($i);
+$i = $i+1;
+Print +s;
+.sort
+
+* Everything should cancel in the end, and we should get zero.
+Identify rat(?a) = rat(?a) - f(rat(?a));
+.sort
+
+PolyRatFun rat;
+.sort
+
+Identify f(x?) = x;
+
+Print;
+.end
+assert succeeded?
+assert result("test") =~ expr("0")
+*--#] Issue336_2 :
 *--#[ Issue340 :
 * Calling "argtoextrasymbol" of a function containing "g5_" crashes
 CF f, g;

sources/poly.cc

@@ -2560,12 +2560,24 @@ const poly poly::argument_to_poly (PHEAD WORD *e, bool with_arghead, bool sort_u
 	}
 
 	res[0] = ri;
-
-	// normalize, since the Form order is probably not the polynomial order
-	// for multiple variables
 
-	if (sort_univar || AN.poly_num_vars>1)
+	// JD: For univariate cases, check whether the ordering is correct or not.
+	// There are various ways to arrive here, but with an incorrect ordering, such
+	// as interactions with collect, moving a polyratfun out of another function
+	// argument, etc.
+	// If the ordering is not correct, make sure we normalize. In multivariate cases,
+	// always normalize as before.
+	if (sort_univar == false && AN.poly_num_vars == 1) {
+		if (res.number_of_terms() >= 2) {
+			if(res[2] < res.last_monomial()[2]) {
+				sort_univar = true;
+			}
+		}
+	}
+
+	if (sort_univar || AN.poly_num_vars>1) {
 		res.normalize();
+	}
 
 	NumberFree(den,"poly::argument_to_poly");
 	NumberFree(pro,"poly::argument_to_poly");

sources/polywrap.cc

@@ -481,7 +481,7 @@ void poly_ratfun_read (WORD *a, poly &num, poly &den) {
 	WORD *astop = a+a[1];
 
 	bool clean = (a[2] & MUSTCLEANPRF) == 0;
-		
+
 	a += FUNHEAD;
 	if (a >= astop) {
 		MLOCK(ErrorMessageLock);
@@ -504,24 +504,32 @@ void poly_ratfun_read (WORD *a, poly &num, poly &den) {
 	else {
 		den = poly(BHEAD 1, modp, 1);
 	}
-	
+
 	if (a < astop) {
 		MLOCK(ErrorMessageLock);
 		MesPrint ((char*)"ERROR: PolyRatFun cannot have more than two arguments");
 		MUNLOCK(ErrorMessageLock);
 		Terminate(-1);
 	}
 
+	// JD: At this point, num and den are certainly sorted into the correct order by
+	// poly::argument_to_poly, but we can't rely on the clean flag to know if there
+	// are any negative powers. Check for them, and set clean = false if there are any.
+	vector<WORD> minpower(AN.poly_num_vars, MAXPOSITIVE);
+
+	for (int i=1; i<num[0]; i+=num[i]) {
+		for (int j=0; j<AN.poly_num_vars; j++) {
+			minpower[j] = MiN(minpower[j], num[i+1+j]);
+		}
+	}
+	for (int i=1; i<den[0]; i+=den[i]) {
+		for (int j=0; j<AN.poly_num_vars; j++) {
+			minpower[j] = MiN(minpower[j], den[i+1+j]);
+			if ( minpower[j] < 0 ) clean = false;
+		}
+	}
+
 	if (!clean) {
-		vector<WORD> minpower(AN.poly_num_vars, MAXPOSITIVE);
-
-		for (int i=1; i<num[0]; i+=num[i])
-			for (int j=0; j<AN.poly_num_vars; j++)
-				minpower[j] = MiN(minpower[j], num[i+1+j]);
-		for (int i=1; i<den[0]; i+=den[i])
-			for (int j=0; j<AN.poly_num_vars; j++)
-				minpower[j] = MiN(minpower[j], den[i+1+j]);
-
 		for (int i=1; i<num[0]; i+=num[i])
 			for (int j=0; j<AN.poly_num_vars; j++)
 				num[i+1+j] -= minpower[j];
@@ -781,6 +789,7 @@ int poly_ratfun_normalize (PHEAD WORD *term) {
 			poly num2(BHEAD 0,modp,1);
 			poly den2(BHEAD 0,modp,1);
 			poly_ratfun_read(t,num2,den2);
+
 			if ((t[2] & MUSTCLEANPRF) != 0) { // first normalize
 				poly gcd1(polygcd::gcd(num2,den2));
 				num2 = num2/gcd1;