Opened 6 years ago

Closed 4 years ago

# Bug in is_hyperbolic

Reported by: Owned by: pkoprowski major sage-8.0 quadratic forms Malcolm Rupert Frédéric Chapoton, Travis Scrimshaw N/A f99393f f99393fc110ed6c5f59136717d5fbf1c69c6c7bd

### 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.

### comment:1 Changed 6 years ago by MRupert

• Branch set to u/MRupert/bug_in_is_hyperbolic

### comment:2 Changed 6 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:

 ​abb9f99 `18430: Fixes self.is_hyperbolic(2) and various cleanups`

### comment:3 follow-up: ↓ 9 Changed 6 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.

```(-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 jdemeyer

• Status changed from needs_review to needs_work

### comment:5 Changed 5 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:

 ​fc968e3 `Merge branch 'u/MRupert/bug_in_is_hyperbolic' in 7.4.b2`

### comment:6 Changed 5 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 4 years ago by chapoton

• Milestone changed from sage-7.4 to sage-8.0

### comment:8 Changed 4 years ago by git

• Commit changed from fc968e3e33e64167ed1364fcf0331d1aa52d2077 to 7690f842f00d3e84b2b6e32c94ee62d924dae09d

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

 ​64cfdf3 `Merge branch 'u/chapoton/18430' in 8.0.b0` ​7690f84 `trac 18430 fixing doctests, plus little code cleanup`

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

I also don't understand why you use `-1` at one point and `"infinity"` somewhere else.

```(-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 git

• Commit changed from 7690f842f00d3e84b2b6e32c94ee62d924dae09d to 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529

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

 ​3231a6d `trac 18430 details`

done

ping ?

### comment:13 Changed 4 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 4 years ago by pmercuri

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

### comment:15 Changed 4 years ago by git

• Commit changed from 3231a6ddc00a7a4049d39ba5a2eacf1d2fdc7529 to f99393fc110ed6c5f59136717d5fbf1c69c6c7bd

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

 ​b4f668a `Merge branch 'u/chapoton/18430' in 8.0.b9` ​f99393f `trac 18430 INPUT field has no dot`

### comment:16 Changed 4 years ago by chapoton

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

### comment:17 Changed 4 years ago by tscrim

• Status changed from needs_review to positive_review

LGTM.

### comment:18 Changed 4 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.