Opened 5 years ago

Closed 3 years ago

#24108 closed enhancement (fixed)

quadratic_form_from_invariants()

Reported by: Anna Haensch Owned by:
Priority: major Milestone: sage-9.0
Component: quadratic forms Keywords: sd90
Cc: Manami Roy, Sandi Rudzinski, Juanita Duque Merged in:
Authors: Simon Brandhorst Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: afa1d73 (Commits, GitHub, GitLab) Commit: afa1d731ca5996c0cc0115bf144ca4b10db3e70d
Dependencies: Stopgaps:

Status badges

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.

Change History (14)

comment:1 Changed 5 years ago by Anna Haensch

Authors: mroy, sbrudzin, jduqueannahaensch, mroy, sbrudzin, jduque

comment:2 Changed 3 years ago by Simon Brandhorst

Branch: u/sbrandhorst/quadratic_form_from_invariants__

comment:3 Changed 3 years ago by Simon Brandhorst

Authors: annahaensch, mroy, sbrudzin, jduqueSimon Brandhorst
Commit: 09526929769530789b71d41c00ba54b3674498cc
Status: newneeds_review

New commits:

0952692QuadraticFormFromInvariants first version

comment:4 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:

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.

comment:5 in reply to:  4 ; Changed 3 years ago by Simon Brandhorst

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?

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

Replying to sbrandhorst:

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.

comment:7 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

Replying to sbrandhorst:

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: 09526929769530789b71d41c00ba54b3674498cc887ca6a38642eb6eea4a8a7692a1b3139e65b112

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

887ca6asmall 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:

887ca6asmall improvements for quadratic_form_from_invariants

comment:11 Changed 3 years ago by Travis Scrimshaw

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

Thank you. One last thing:

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

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

comment:12 Changed 3 years ago by git

Commit: 887ca6a38642eb6eea4a8a7692a1b3139e65b112afa1d731ca5996c0cc0115bf144ca4b10db3e70d

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

afa1d73docs

comment:13 Changed 3 years ago by Simon Brandhorst

Status: needs_reviewpositive_review

comment:14 Changed 3 years ago by Volker Braun

Branch: u/sbrandhorst/quadratic_form_from_invariants__afa1d731ca5996c0cc0115bf144ca4b10db3e70d
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.