Fix Singular polynomial comparison

[user202729]

Fixes #35681

Basically, I think this change makes the comparison makes more sense (apart from that, it should speed up the comparison a bit because it only calls cfGreater when cfEqual returns False, and get rid of the cfSub)

After this change, as long as cfGreater defines a total order, you get a total order on the polynomials.
For example now Integers(10)[] has a total order.

For unordered fields however, cfGreater may not define a total order anyway;
but in newer Singular, cfGreater now defines a total order. It's not always consistent with cfGreaterZero though.
Singular/Singular#1253 (comment)

There's a lot of test breakage because now we change which polynomial compares greater than 0, and that is used to determine whether the sign to display is + or - in several places.

---

More detail:

    TESTS::

        sage: R.<x, y> = ZZ[]
        sage: x^2+x > x^2
        True
        sage: x^2 > x^2-x                       # NEW
        True
        sage: x^2 > x^2-x                       # OLD
        False
        sage: x^2+x > x^2-x
        True
        sage: x^2 > 0
        True
        sage: -x^2 > 0                          # NEW
        False
        sage: -x^2 > 0                          # OLD
        True
        sage: x^2 > -x^2
        True

    ::

        sage: R.<x, y> = GF(7)[]
        sage: x^2+x > x^2
        True
        sage: x^2 > x^2-x                       # NEW
        True
        sage: x^2 > x^2-x                       # OLD
        False
        sage: x^2+x > x^2-x                     # NEW
        False
        sage: x^2+x > x^2-x                     # OLD
        True
        sage: x^2 > 0
        True
        sage: -x^2 > 0                          # NEW
        False
        sage: -x^2 > 0                          # OLD
        True
        sage: x^2 > -x^2                        # NEW
        False
        sage: x^2 > -x^2                        # OLD
        True

    ::

        sage: R.<x, y> = Integers(8)[]
        sage: x^2+x > x^2
        True
        sage: x^2 > x^2-x                       # NEW
        True
        sage: x^2 > x^2-x                       # OLD
        False
        sage: x^2+x > x^2-x
        True
        sage: x^2 > 0
        True
        sage: -x^2 > 0                          # NEW
        False
        sage: -x^2 > 0                          # OLD
        True
        sage: x^2 > -x^2
        True

    ::

        sage: R.<x, y> = Integers(10)[]
        sage: x^2+x > x^2
        True
        sage: x^2 > x^2-x
        False
        sage: x^2+x > x^2-x                     # NEW
        False
        sage: x^2+x > x^2-x                     # OLD
        True
        sage: x^2 > 0
        True
        sage: -x^2 > 0
        True
        sage: x^2 > -x^2                        # NEW
        False
        sage: x^2 > -x^2                        # OLD
        True

    ::

        sage: R.<x, y> = ZZ[]
        sage: l = [i*x+j*x^2 for i in range(-5, 5) for j in range(-5, 5)]
        sage: l.sort()
        sage: for i in range(len(l)):
        ....:     for b in l[:i]:
        ....:         assert b < l[i], (b, l[i])

    ::

        sage: R.<x, y> = Integers(10)[]
        sage: l = [i*x+j*y for i in range(7) for j in range(7)]
        sage: l.sort()
        sage: for i in range(len(l)):           # NEW
        ....:     for b in l[:i]:
        ....:         assert b < l[i], (b, l[i])
        sage: for i in range(len(l)):           # OLD
        ....:     for b in l[:i]:
        ....:         assert b < l[i], (b, l[i])
        Traceback (most recent call last):
        ...
        AssertionError: (y, 2*y)

The comparison function was added all the way back in 2007 without explanation e22dc86

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

[github-actions]

Documentation preview for this PR (built with commit f18e9fc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[kwankyu]

I like this change. Would you resolve the conflicts?

Moreover, I think we should not lose existing elaborate formatting for PEP8, in particular splitting long lines.

[kwankyu]

sage: R.<x, y> = Integers(10)[]
sage: l = [i*x+j*y for i in range(7) for j in range(7)]
sage: l.sort()
sage: for i in range(len(l)):           # NEW
....:     for b in l[:i]:
....:         assert b < l[i], (b, l[i])

How about adding this doctest somewhere?

[user202729]

What I mean is the long lines are inserted there by sage --fixdoctests… Anyway I wrote a quick script to do the reformatting automatically.

"""
Script to reformat diff file to wrap lines automatically

usage:

	git diff develop > /tmp/a.diff
	python [this script] < /tmp/a.diff > /tmp/b.diff
	git apply -R /tmp/a.diff
	git apply /tmp/b.diff
"""

import textwrap
import re
import sys

def reformat_diff(input_diff):
	lines = input_diff.split('\n')
	output_lines = []

	for line in lines:
		if line.startswith('+') and not line.startswith('+++'):
			leading_spaces = re.match(r'^\+(\s*)', line).group(1)
			stripped_line = line[len(leading_spaces) + 1:]
			if not stripped_line:
				output_lines.append(line)
			else:
				wrapped_lines = textwrap.wrap(stripped_line, width=120,
											  initial_indent=f"+{leading_spaces}",
											  subsequent_indent=f"+ {leading_spaces}",	# one added space here
											  break_long_words=False,
											  break_on_hyphens=False,
											  )
				for wrapped_line in wrapped_lines:
					output_lines.append(wrapped_line)
		else:
			output_lines.append(line)

	return '\n'.join(output_lines)

import sys
import subprocess

input_diff = sys.stdin.read()

output_diff = reformat_diff(input_diff)

result = subprocess.run(
    ['recountdiff'],
    input=output_diff,
    text=True,
	encoding='u8',
    capture_output=True
)

print(result.stdout, end='')

if result.stderr:
    print(result.stderr, file=sys.stderr)

This should probably be incorporated into sage-fixdoctests eventually.

The issues should be fixed now, and tests on changed files pass locally.

[kwankyu]

Thanks for explanation.

sage-fixdoctests does not touch the (manually formatted) output if the doctest passes. The output line get changed (and the manual formatting is gone) only when the doctest fails to pass. In the latter case, manual formatting should be redone.

I think there is nothing wrong with the procedure. Manual formatting would be rarely needed. The present case is special since by the very nature of the goal many doctests fail and sage-fixdoctest eradicates manual formatting.

We may develop an automatic formatter (for doctest outputs) as you attempted above. But I think it would be very hard to make it perfect. If the automatic formatter is not perfect, then we would need to examine each of the changes it makes anyway. I doubt that developing it is worth while...

[kwankyu]

I prepared a commit in the branch https://github.com/kwankyu/sage/tree/p/recover-formatting that recovers the manual formatting to your branch.

I followed PEP8 that prefers the style like

123
+ 456 
+ 789

and tried to format expressions to make them look like the original.

Could you cherry-pick the last commit of the branch?

[user202729]

Sure, thanks for the effort.

[kwankyu]

Thanks. This PR is good to go once all checks pass.

[user202729]

sage: R.<x, y> = Integers(10)[]
sage: l = [i*x+j*y for i in range(7) for j in range(7)]
sage: l.sort()
sage: for i in range(len(l)):           # NEW
....:     for b in l[:i]:
....:         assert b < l[i], (b, l[i])

How about adding this doctest somewhere?

I forget to apply this, fixed now.

Also fix a broken test (you probably didn't notice it because you need --long to see the test fail). Originally (without this pull request) that one has long line as well (it's the author intention I suppose, although we can replace that with trailing ..., with the risk of it matching too much)

[kwankyu]

Also fix a broken test (you probably didn't notice it because you need --long to see the test fail). Originally (without this pull request) that one has long line as well (it's the author intention I suppose, although we can replace that with trailing ..., with the risk of it matching too much)

Ah right. Thanks.

[kwankyu]

Thanks. LGTM.