Check for duplicate hyperplanes in arrangements over any base ring

[Julian Ritter]

A hyperplane arrangement is supposed to be checked for duplicate hyperplanes. This works well in QQ:

sage: H1.<x,y> = HyperplaneArrangements(base_ring=QQ)
sage: A = H1([1,1,0],[2,2,0], warn_duplicates=True)
/usr/local/sagemath/dev/local/lib/python3.7/site-packages/sage/geometry/hyperplane_arrangement/arrangement.py:3352: UserWarning: Input contained 2 hyperplanes, but only 1 are distinct.
  warn('Input contained {0} hyperplanes, but only {1} are distinct.'.format(n, len(hyperplanes)))
sage: A
Arrangement <x + 1>

sage: A.n_regions()
2

In other base rings, this check can fail, resulting in false statements about the arrangement:

sage: R = QQ[sqrt(2)]
sage: H2.<x,y> = HyperplaneArrangements(base_ring=R)
sage: B = H2([1,1,0],[2,2,0], warn_duplicates=True)
sage: B
Arrangement <x + 1 | 2*x + 2>

sage: B.n_regions()
3

[Julian Ritter]

The error can be tracked down to the primitive method for hyperplanes: It is supposed to rescale linear multiples of hyperplane directions to a canonical "primitive" one. This is currently not properly implied for every base_ring, as it uses gcd and lcm which for some base rings are just one regardless of the arguments.

Furthermore, the method currently uses x.denom() and x.numer() instead of x.denominator() and x.numerator(), two aliases which are only defined for QQ.

One solution would be to normalize the hyperplane equations instead of working with gcd and lcm. But this gives "ugly" equations, contrary to what the primitive method is meant to provide.

[Matthias Köppe]

New commits:

​49db0e3

Replace aliases by full method names

[Matthias Köppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[Matthias Köppe]

Setting a new milestone for this ticket based on a cursory review.

[Matthias Köppe]

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​216f6df

Change behavior of primitive() for rings other than QQ

[Julian Ritter]

I kept the existing procedure of primitive() for QQ (using numerator, denominator, gcd and lcm) and introduced a new one for other base rings: The linear expression is scaled such that the first nonzero entry of the normal vector is one or negative one (as before, depending on the original sign and whether the option signed is set).

This seems to be compatible with all examples provided in the classes concerned, pass all the existing doctests in the hyperplane_arrangements folder and solve the problem of this ticket. It should also solve ticket #30749. I included suitable tests in the code.

Note however that this does change the coefficients by which hyperplanes are stored in an arrangement not based on QQ. This could break user code whenever it relies on a specific scaling of the linear expression. I would argue that fixing the mathematically incorrect statements yielded by the current implementation outweighs this inconvenience.

[Matthias Köppe]

+        from sage.rings.all import QQ

Please use a more specific import, see ​https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies

[Matthias Köppe]

Likewise here:

+            from sage.arith.all import lcm, gcd

[Matthias Köppe]

Also, it would be good if you could rewrite these doctests so that they do not go through SR.

+        Check that :trac:`30078` is fixed::
+
+            sage: R = QQ[sqrt(2)]

[Matthias Köppe]

(This is for modularization purposes, see #32432)

[Matthias Köppe]

Also the list brackets can be omitted here:

+            d = lcm([x.denominator() for x in coeffs])
+            n = gcd([x.numerator() for x in coeffs])

[Matthias Köppe]

Also ./sage -tox -e pycodestyle src/sage/geometry/hyperplane_arrangement/hyperplane.py complains about style, some of the warnings are on the code changed in the ticket

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​cecc4b5	More specific imports
​1778600	Simplified doctests
​63a2b4e	pycodestyle cleanup

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​bec2e96

Limit pycodestyle cleanup to the method modified in this ticket

[Julian Ritter]

Thanks for the helpful suggestions!

Replying to mkoeppe:

Also, it would be good if you could rewrite these doctests so that they do not go through SR.

+        Check that :trac:`30078` is fixed::
+
+            sage: R = QQ[sqrt(2)]

I am not really sure about what goes through SR and what doesn’t. For the tests to be meaningful, the base ring would have to contain some non-rationals. I replaced QQ[sqrt(2)] by AA. Is that fine?

[Matthias Köppe]

sage: ((1+sqrt(5))/2).parent()
Symbolic Ring
sage: ((1+AA(5).sqrt())/2).parent()
Algebraic Real Field

[Matthias Köppe]

and instead of QQ[sqrt(2)] you can write

sage: QuadraticField(2)
Number Field in a with defining polynomial x^2 - 2 with a = 1.414213562373095?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​c1f4173

Avoid computations using Symbolic Ring in doctests

[Julian Ritter]

Replying to mkoeppe:

and instead of QQ[sqrt(2)] you can write

sage: QuadraticField(2)
Number Field in a with defining polynomial x^2 - 2 with a = 1.414213562373095?

Thanks, I didn’t know that!

[Matthias Köppe]

+            sage: R = QuadraticField(2)
+            sage: H.<x,y> = HyperplaneArrangements(base_ring=R)
+            sage: B = H([1,1,0], [2,2,0], [sqrt(2),sqrt(2),0])

Make that R.<sqrt2> = QuadraticField(2) and use sqrt2 instead of sqrt(2)

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​b7f7a7f

Correct variable name in quadratic field

[Julian Ritter]

Replying to mkoeppe:

+            sage: R = QuadraticField(2)
+            sage: H.<x,y> = HyperplaneArrangements(base_ring=R)
+            sage: B = H([1,1,0], [2,2,0], [sqrt(2),sqrt(2),0])

Make that R.<sqrt2> = QuadraticField(2) and use sqrt2 instead of sqrt(2)

Oh, I see. When sqrt(2) is used, it will initially be a symbolic expression and turned into an element of R later, which can be avoided by explicitly using the element of the quadratic field, correct?

[Matthias Köppe]

That's right.