Opened 5 years ago

Closed 3 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) 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 5 years ago by MRupert

  • Branch set to u/MRupert/bug_in_is_hyperbolic

comment:2 Changed 5 years ago by MRupert

  • Authors set to Malcolm Rupert
  • Commit set to abb9f998469824499d2ac1c26234363c6b6a2a72
  • Status changed from new to needs_review

This commit should fix the problem. I also added functionality and improved the documentation on the infinite place.


New commits:

abb9f9918430: Fixes self.is_hyperbolic(2) and various cleanups

comment:3 follow-up: Changed 5 years ago by jdemeyer

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 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:5 Changed 4 years ago by chapoton

  • Branch changed from u/MRupert/bug_in_is_hyperbolic to u/chapoton/18430
  • Commit changed from abb9f998469824499d2ac1c26234363c6b6a2a72 to fc968e3e33e64167ed1364fcf0331d1aa52d2077

rebase and clean-up


New commits:

fc968e3Merge branch 'u/MRupert/bug_in_is_hyperbolic' in 7.4.b2

comment:6 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.7 to sage-7.4
  • Status changed from needs_work to needs_review

comment:7 Changed 3 years ago by chapoton

  • Milestone changed from sage-7.4 to sage-8.0

comment:8 Changed 3 years ago by git

  • Commit changed from fc968e3e33e64167ed1364fcf0331d1aa52d2077 to 7690f842f00d3e84b2b6e32c94ee62d924dae09d

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

64cfdf3Merge branch 'u/chapoton/18430' in 8.0.b0
7690f84trac 18430 fixing doctests, plus little code cleanup

comment:9 in reply to: ↑ 3 Changed 3 years ago by tscrim

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 3 years ago by git

  • Commit changed from 7690f842f00d3e84b2b6e32c94ee62d924dae09d to 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529

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

3231a6dtrac 18430 details

comment:11 Changed 3 years ago by chapoton

done

comment:12 Changed 3 years ago by chapoton

ping ?

comment:13 Changed 3 years ago by tscrim

  • 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 pmercuri

  • Status changed from needs_review to needs_work
  • Work issues set to documentation formatting

comment:15 Changed 3 years ago by git

  • Commit changed from 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529 to f99393fc110ed6c5f59136717d5fbf1c69c6c7bd

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

b4f668aMerge branch 'u/chapoton/18430' in 8.0.b9
f99393ftrac 18430 INPUT field has no dot

comment:16 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review
  • Work issues documentation formatting deleted

comment:17 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:18 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/18430 to f99393fc110ed6c5f59136717d5fbf1c69c6c7bd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.