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:  sage9.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: 
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
Authors:  mroy, sbrudzin, jduque → annahaensch, mroy, sbrudzin, jduque 

comment:2 Changed 3 years ago by
Branch:  → u/sbrandhorst/quadratic_form_from_invariants__ 

comment:3 Changed 3 years ago by
Authors:  annahaensch, mroy, sbrudzin, jduque → Simon Brandhorst 

Commit:  → 09526929769530789b71d41c00ba54b3674498cc 
Status:  new → needs_review 
comment:4 followup: 5 Changed 3 years ago by
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 followup: 6 Changed 3 years ago by
Replying to tscrim:
A few little things: Why is a function
QuadraticFormFromInvariants
following class definition CamelCase and not the usingquadratic_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 Changed 3 years ago by
Replying to sbrandhorst:
Replying to tscrim:
A few little things: Why is a function
QuadraticFormFromInvariants
following class definition CamelCase and not the usingquadratic_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 viaQuadraticForm
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 followup: 8 Changed 3 years ago by
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 Changed 3 years ago by
Replying to sbrandhorst:
What I mean is to give the parameters as input to the constructor of the
QuadraticForm
class. So the syntax would beQuadraticform(QQ,invariants=[....])
orQuadraticform(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 goto/use.
comment:9 Changed 3 years ago by
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
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
Milestone:  sage8.1 → sage9.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
Commit:  887ca6a38642eb6eea4a8a7692a1b3139e65b112 → afa1d731ca5996c0cc0115bf144ca4b10db3e70d 

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

comment:13 Changed 3 years ago by
Status:  needs_review → positive_review 

comment:14 Changed 3 years ago by
Branch:  u/sbrandhorst/quadratic_form_from_invariants__ → afa1d731ca5996c0cc0115bf144ca4b10db3e70d 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
QuadraticFormFromInvariants first version