quadratic_form_from_invariants()

[annahaensch]

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.

CC: @sagetrac-mroy @sagetrac-sbrudzin @sagetrac-jduque

Component: quadratic forms

Keywords: sd90

Author: Simon Brandhorst

Branch/Commit: afa1d73

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/24108

[annahaensch]

Changed author from mroy, sbrudzin, jduque to annahaensch, mroy, sbrudzin, jduque

[simonbrandhorst]

Branch: u/sbrandhorst/quadratic_form_from_invariants__

[simonbrandhorst]

Changed author from annahaensch, mroy, sbrudzin, jduque to Simon Brandhorst

[simonbrandhorst]

New commits:

0952692

QuadraticFormFromInvariants first version

[simonbrandhorst]

Commit: 0952692

[tscrim]

comment:4

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:

Error messages should start with a lowercase letter.

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.

[simonbrandhorst]

comment:5

Replying to @tscrim:

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?

[tscrim]

comment:6

Replying to @simonbrandhorst:

Replying to @tscrim:

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.

[simonbrandhorst]

comment:7

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?

[tscrim]

comment:8

Replying to @simonbrandhorst:

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.

[sagetrac-git]

Changed commit from 0952692 to 887ca6a

[sagetrac-git]

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

887ca6a

small improvements for quadratic_form_from_invariants

[simonbrandhorst]

comment:10

I did it as quadratic_form_from_invariants.

---

New commits:

887ca6a

small improvements for quadratic_form_from_invariants

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:11

Thank you. One last thing:

-    ALGORITHM::
+    ALGORITHM:
 
-        We follow [Kir2016]_.
+    We follow [Kir2016]_.

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

[sagetrac-git]

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

afa1d73

docs

[sagetrac-git]

Changed commit from 887ca6a to afa1d73

[vbraun]

Changed branch from u/sbrandhorst/quadratic_form_from_invariants__ to afa1d73