Opened 5 years ago
Closed 3 years ago
#18430 closed defect (fixed)
Bug in is_hyperbolic
Reported by:  pkoprowski  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  quadratic forms  Keywords:  
Cc:  Merged in:  
Authors:  Malcolm Rupert  Reviewers:  Frédéric Chapoton, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  f99393f (Commits)  Commit:  f99393fc110ed6c5f59136717d5fbf1c69c6c7bd 
Dependencies:  Stopgaps: 
Description
The method is_hyperbolic in the class of quadratic form returns incorrect results over the field QQ_2. Here is an explicit example:
q = DiagonalQuadraticForm(QQ, [1,1,1,1]) print q.is_hyperbolic(2)
The form <1,1,1,1> is clearly hyperbolic  by definition. It is a sum of two hyperbolic planes. Nevertheless Sage returns False
here.
The reason is as follows. In the file quadratic_forms/quadratic_form__local_field_invariants.py
in function is_hyperbolic
the actual code is
elif p == 2: return QQ(self.det() * (1)**m).is_padic_square(p) and (self.hasse_invariant(p) == (1)**m) ## Actually, this 1 is the Hilbert symbol (1,1)_p
while it should be
elif p == 2: return QQ(self.det() * (1)**m).is_padic_square(p) and (self.hasse_invariant(p) == (1)**(m*(m1)/2)) ## Actually, this 1 is the Hilbert symbol (1,1)_p
For mathematical explanation see e.g. T.Y. Lam "Introduction to Quadratic Forms over Field", Proposition V.3.25
I'm not sure how to formally patch the code, so I'm posting it this way.
Change History (18)
comment:1 Changed 5 years ago by
 Branch set to u/MRupert/bug_in_is_hyperbolic
comment:2 Changed 5 years ago by
 Commit set to abb9f998469824499d2ac1c26234363c6b6a2a72
 Status changed from new to needs_review
comment:3 followup: ↓ 9 Changed 5 years ago by
Can you replace
if p == "infinity": return self.is_definite() else: ...
by
if p == Infinity: return self.is_definite() ...
This means you don't need to indent the whole block for p
a prime number and I also prefer the actual value Infinity
(which needs to be imported from sage.rings.infinity
) instead of the string "infinity"
.
I also don't understand why you use 1
at one point and "infinity"
somewhere else.
And instead of writing
(1)**(m*(m1)/2)) ## Actually, this 1 is the Hilbert symbol (1,1)
why don't you actually write
hilbert_symbol(1, 1, p)**(m*(m1)/2))
comment:4 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:5 Changed 4 years ago by
 Branch changed from u/MRupert/bug_in_is_hyperbolic to u/chapoton/18430
 Commit changed from abb9f998469824499d2ac1c26234363c6b6a2a72 to fc968e3e33e64167ed1364fcf0331d1aa52d2077
comment:6 Changed 4 years ago by
 Milestone changed from sage6.7 to sage7.4
 Status changed from needs_work to needs_review
comment:7 Changed 3 years ago by
 Milestone changed from sage7.4 to sage8.0
comment:8 Changed 3 years ago by
 Commit changed from fc968e3e33e64167ed1364fcf0331d1aa52d2077 to 7690f842f00d3e84b2b6e32c94ee62d924dae09d
comment:9 in reply to: ↑ 3 Changed 3 years ago by
Replying to jdemeyer:
I also don't understand why you use
1
at one point and"infinity"
somewhere else.And instead of writing
(1)**(m*(m1)/2)) ## Actually, this 1 is the Hilbert symbol (1,1)why don't you actually write
hilbert_symbol(1, 1, p)**(m*(m1)/2))
I am pretty sure the former is (much) faster and the comment makes the simplification clear.
comment:10 Changed 3 years ago by
 Commit changed from 7690f842f00d3e84b2b6e32c94ee62d924dae09d to 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529
Branch pushed to git repo; I updated commit sha1. New commits:
3231a6d  trac 18430 details

comment:11 Changed 3 years ago by
done
comment:12 Changed 3 years ago by
ping ?
comment:13 Changed 3 years ago by
 Reviewers set to Frédéric Chapoton, Travis Scrimshaw
If you just fix up this INPUT:
docstring (in a few places):
`p`  a prime number > 0 or `1` for the infinite place. + `p`  a prime number > 0 or `1` for the infinite place
Once that is done, you can set a positive review on my behalf.
comment:14 Changed 3 years ago by
 Status changed from needs_review to needs_work
 Work issues set to documentation formatting
comment:15 Changed 3 years ago by
 Commit changed from 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529 to f99393fc110ed6c5f59136717d5fbf1c69c6c7bd
comment:16 Changed 3 years ago by
 Status changed from needs_work to needs_review
 Work issues documentation formatting deleted
comment:18 Changed 3 years ago by
 Branch changed from u/chapoton/18430 to f99393fc110ed6c5f59136717d5fbf1c69c6c7bd
 Resolution set to fixed
 Status changed from positive_review to closed
This commit should fix the problem. I also added functionality and improved the documentation on the infinite place.
New commits:
18430: Fixes self.is_hyperbolic(2) and various cleanups