Opened 6 years ago
Closed 4 years ago
#18430 closed defect (fixed)
Bug in is_hyperbolic
Reported by: | pkoprowski | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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, GitHub, GitLab) | 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*(m-1)/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 6 years ago by
- Branch set to u/MRupert/bug_in_is_hyperbolic
comment:2 Changed 6 years ago by
- Commit set to abb9f998469824499d2ac1c26234363c6b6a2a72
- Status changed from new to needs_review
comment:3 follow-up: ↓ 9 Changed 6 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*(m-1)/2)) ## Actually, this -1 is the Hilbert symbol (-1,-1)
why don't you actually write
hilbert_symbol(-1, -1, p)**(m*(m-1)/2))
comment:4 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:5 Changed 5 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 5 years ago by
- Milestone changed from sage-6.7 to sage-7.4
- Status changed from needs_work to needs_review
comment:7 Changed 4 years ago by
- Milestone changed from sage-7.4 to sage-8.0
comment:8 Changed 4 years ago by
- Commit changed from fc968e3e33e64167ed1364fcf0331d1aa52d2077 to 7690f842f00d3e84b2b6e32c94ee62d924dae09d
comment:9 in reply to: ↑ 3 Changed 4 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*(m-1)/2)) ## Actually, this -1 is the Hilbert symbol (-1,-1)why don't you actually write
hilbert_symbol(-1, -1, p)**(m*(m-1)/2))
I am pretty sure the former is (much) faster and the comment makes the simplification clear.
comment:10 Changed 4 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 4 years ago by
done
comment:12 Changed 4 years ago by
ping ?
comment:13 Changed 4 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 4 years ago by
- Status changed from needs_review to needs_work
- Work issues set to documentation formatting
comment:15 Changed 4 years ago by
- Commit changed from 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529 to f99393fc110ed6c5f59136717d5fbf1c69c6c7bd
comment:16 Changed 4 years ago by
- Status changed from needs_work to needs_review
- Work issues documentation formatting deleted
comment:18 Changed 4 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