Opened 2 years ago
Closed 3 months ago
#30078 closed defect (fixed)
Check for duplicate hyperplanes in arrangements over any base ring
Reported by: | nailuj | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.6 |
Component: | geometry | Keywords: | HyperplaneArrangements |
Cc: | jipilab | Merged in: | |
Authors: | Julian Ritter | Reviewers: | Matthias Koeppe |
Report Upstream: | N/A | Work issues: | |
Branch: | b7f7a7f (Commits, GitHub, GitLab) | Commit: | b7f7a7f44f71ff49e0551b76db5d01d44d500d2d |
Dependencies: | Stopgaps: |
Description
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
Change History (29)
comment:1 Changed 2 years ago by
- Status changed from new to needs_info
comment:2 Changed 2 years ago by
- Branch set to u/nailuj/check_for_duplicate_hyperplanes_in_arrangements_over_any_base_ring
comment:3 Changed 2 years ago by
- Cc jipilab added
- Commit set to 49db0e34eb135c0ecef7b3b09ee2760ae099d8da
New commits:
49db0e3 | Replace aliases by full method names
|
comment:4 Changed 20 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:5 Changed 16 months ago by
- Milestone changed from sage-9.3 to sage-9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:6 Changed 11 months ago by
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
comment:7 Changed 6 months ago by
- Milestone changed from sage-9.5 to sage-9.6
Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:8 Changed 3 months ago by
- Commit changed from 49db0e34eb135c0ecef7b3b09ee2760ae099d8da to 216f6df74100fc78ebf05fd84696d927eb14114d
Branch pushed to git repo; I updated commit sha1. New commits:
216f6df | Change behavior of primitive() for rings other than QQ
|
comment:9 Changed 3 months ago by
- Status changed from needs_info to needs_review
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.
comment:10 Changed 3 months ago by
+ 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
comment:11 Changed 3 months ago by
Likewise here:
+ from sage.arith.all import lcm, gcd
comment:12 follow-up: ↓ 19 Changed 3 months ago by
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)]
comment:13 Changed 3 months ago by
(This is for modularization purposes, see #32432)
comment:14 Changed 3 months ago by
Also the list brackets can be omitted here:
+ d = lcm([x.denominator() for x in coeffs]) + n = gcd([x.numerator() for x in coeffs])
comment:15 Changed 3 months ago by
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
comment:16 Changed 3 months ago by
- Status changed from needs_review to needs_work
comment:17 Changed 3 months ago by
- Commit changed from 216f6df74100fc78ebf05fd84696d927eb14114d to 63a2b4ea5e8ceeb49f3f7ae41a21931545124ed8
comment:18 Changed 3 months ago by
- Commit changed from 63a2b4ea5e8ceeb49f3f7ae41a21931545124ed8 to bec2e96b3547e44518149073b94a1284610355ac
Branch pushed to git repo; I updated commit sha1. New commits:
bec2e96 | Limit pycodestyle cleanup to the method modified in this ticket
|
comment:19 in reply to: ↑ 12 Changed 3 months ago by
- Status changed from needs_work to needs_review
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?
comment:20 Changed 3 months ago by
sage: ((1+sqrt(5))/2).parent() Symbolic Ring sage: ((1+AA(5).sqrt())/2).parent() Algebraic Real Field
comment:21 follow-up: ↓ 23 Changed 3 months ago by
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?
comment:22 Changed 3 months ago by
- Commit changed from bec2e96b3547e44518149073b94a1284610355ac to c1f4173bf8dcfdd7e1b9bb899f58f38ef8e54db1
Branch pushed to git repo; I updated commit sha1. New commits:
c1f4173 | Avoid computations using Symbolic Ring in doctests
|
comment:23 in reply to: ↑ 21 Changed 3 months ago by
Replying to mkoeppe:
and instead of
QQ[sqrt(2)]
you can writesage: QuadraticField(2) Number Field in a with defining polynomial x^2 - 2 with a = 1.414213562373095?
Thanks, I didn’t know that!
comment:24 follow-up: ↓ 26 Changed 3 months ago by
+ 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)
comment:25 Changed 3 months ago by
- Commit changed from c1f4173bf8dcfdd7e1b9bb899f58f38ef8e54db1 to b7f7a7f44f71ff49e0551b76db5d01d44d500d2d
Branch pushed to git repo; I updated commit sha1. New commits:
b7f7a7f | Correct variable name in quadratic field
|
comment:26 in reply to: ↑ 24 Changed 3 months ago by
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 usesqrt2
instead ofsqrt(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?
comment:27 Changed 3 months ago by
That's right.
comment:28 Changed 3 months ago by
- Reviewers set to Matthias Koeppe
- Status changed from needs_review to positive_review
comment:29 Changed 3 months ago by
- Branch changed from u/nailuj/check_for_duplicate_hyperplanes_in_arrangements_over_any_base_ring to b7f7a7f44f71ff49e0551b76db5d01d44d500d2d
- Resolution set to fixed
- Status changed from positive_review to closed
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 everybase_ring
, as it usesgcd
andlcm
which for some base rings are just one regardless of the arguments.Furthermore, the method currently uses
x.denom()
andx.numer()
instead ofx.denominator()
andx.numerator()
, two aliases which are only defined forQQ
.One solution would be to normalize the hyperplane equations instead of working with
gcd
andlcm
. But this gives "ugly" equations, contrary to what theprimitive
method is meant to provide.