Opened 5 years ago

Closed 3 years ago

## #24108 closed enhancement (fixed)

Reported by: Owned by: Anna Haensch major sage-9.0 quadratic forms sd90 Manami Roy, Sandi Rudzinski, Juanita Duque Simon Brandhorst Travis Scrimshaw N/A afa1d73 afa1d731ca5996c0cc0115bf144ca4b10db3e70d

### Description

This is algorithm 3.4.3 from Markus Kirschmer's "Definite quadratic and hermitian forms with small class number" It returns a global quadratic form corresponding to a given set of local invariants.

### comment:1 Changed 5 years ago by Anna Haensch

Authors: mroy, sbrudzin, jduque → annahaensch, mroy, sbrudzin, jduque

### comment:3 Changed 3 years ago by Simon Brandhorst

Authors: annahaensch, mroy, sbrudzin, jduque → Simon Brandhorst → 09526929769530789b71d41c00ba54b3674498cc new → needs_review

New commits:

 ​0952692 `QuadraticFormFromInvariants first version`

### comment:4 follow-up:  5 Changed 3 years ago by Travis Scrimshaw

A few little things:

```-    Let `(a_1,...,a_n)` be the gram marix of a regular quadratic space.
+    Let `(a_1, \ldots, a_n)` be the gram marix of a regular quadratic space.
Then Cassel's Hasse invariant is defined as

.. MATH::

-        \prod_{i<j} (a_i,a_j)
+        \prod_{i<j} (a_i,a_j),

where `(a_i,a_j)` denotes the Hilbert symbol.
```

Should `gram` be capitalized to `Gram`?

`if F!=QQ:` -> `if F is not QQ:`

The `assert all()` does not need the list comprehension, so you just need

```assert all((a*d).valuation(p) % 2 == 1 for p in Pprime)
```

Why is a function `QuadraticFormFromInvariants` following class definition CamelCase and not the using `quadratic_form_from_invariants`? It is not constructing a particular class.

### comment:5 in reply to:  4 ; follow-up:  6 Changed 3 years ago by Simon Brandhorst

A few little things: Why is a function `QuadraticFormFromInvariants` following class definition CamelCase and not the using `quadratic_form_from_invariants`? It is not constructing a particular class.

I believe that in this way it is easier to discover. With `QuadraticForm` + autocomplete. An alternative would be to access this functionality via `QuadraticForm` directly?

### comment:6 in reply to:  5 Changed 3 years ago by Travis Scrimshaw

A few little things: Why is a function `QuadraticFormFromInvariants` following class definition CamelCase and not the using `quadratic_form_from_invariants`? It is not constructing a particular class.

I believe that in this way it is easier to discover. With `QuadraticForm` + autocomplete. An alternative would be to access this functionality via `QuadraticForm` directly?

To me, that is not a good enough reason to break convention (in fact, to me and how I normally tell people to find via autocomplete, this would be harder to find because I know it would be a function). The other alternative would be as you said: attach it as either a `@staticmethod` or a method to `QuadraticForm`.

### comment:7 follow-up:  8 Changed 3 years ago by Simon Brandhorst

What I mean is to give the parameters as input to the constructor of the `QuadraticForm` class. So the syntax would be `Quadraticform(QQ,invariants=[....])` or `Quadraticform(QQ,rk,det,P,signatures)`? After all we are constructing a single quadratic form from some input?

### comment:8 in reply to:  7 Changed 3 years ago by Travis Scrimshaw

What I mean is to give the parameters as input to the constructor of the `QuadraticForm` class. So the syntax would be `Quadraticform(QQ,invariants=[....])` or `Quadraticform(QQ,rk,det,P,signatures)`? After all we are constructing a single quadratic form from some input?

Either one of those options would be much better IMO. It also means there is precisely one place in the documentation and code that people should go-to/use.

### comment:9 Changed 3 years ago by git

Commit: 09526929769530789b71d41c00ba54b3674498cc → 887ca6a38642eb6eea4a8a7692a1b3139e65b112

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

 ​887ca6a `small improvements for quadratic_form_from_invariants`

### comment:10 Changed 3 years ago by Simon Brandhorst

I did it as `quadratic_form_from_invariants`.

New commits:

 ​887ca6a `small improvements for quadratic_form_from_invariants`

### comment:11 Changed 3 years ago by Travis Scrimshaw

Milestone: sage-8.1 → sage-9.0 → Travis Scrimshaw

Thank you. One last thing:

```-    ALGORITHM::
+    ALGORITHM:

```

Once done, you can set a positive review on my behalf.

### comment:12 Changed 3 years ago by git

Commit: 887ca6a38642eb6eea4a8a7692a1b3139e65b112 → afa1d731ca5996c0cc0115bf144ca4b10db3e70d

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

 ​afa1d73 `docs`

### comment:13 Changed 3 years ago by Simon Brandhorst

Status: needs_review → positive_review

### comment:14 Changed 3 years ago by Volker Braun

Branch: u/sbrandhorst/quadratic_form_from_invariants__ → afa1d731ca5996c0cc0115bf144ca4b10db3e70d → fixed positive_review → closed
Note: See TracTickets for help on using tickets.