Opened 4 years ago
Closed 23 months ago
#24108 closed enhancement (fixed)
quadratic_form_from_invariants()
Reported by:  annahaensch  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  quadratic forms  Keywords:  sd90 
Cc:  mroy, sbrudzin, jduque  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 4 years ago by
comment:2 Changed 2 years ago by
 Branch set to u/sbrandhorst/quadratic_form_from_invariants__
comment:3 Changed 2 years ago by
 Commit set to 09526929769530789b71d41c00ba54b3674498cc
 Status changed from new to needs_review
comment:4 followup: ↓ 5 Changed 23 months 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 in reply to: ↑ 4 ; followup: ↓ 6 Changed 23 months 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 in reply to: ↑ 5 Changed 23 months 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 23 months 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 in reply to: ↑ 7 Changed 23 months 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 23 months ago by
 Commit changed from 09526929769530789b71d41c00ba54b3674498cc to 887ca6a38642eb6eea4a8a7692a1b3139e65b112
Branch pushed to git repo; I updated commit sha1. New commits:
887ca6a  small improvements for quadratic_form_from_invariants

comment:10 Changed 23 months ago by
I did it as quadratic_form_from_invariants
.
New commits:
887ca6a  small improvements for quadratic_form_from_invariants

comment:11 Changed 23 months ago by
 Milestone changed from sage8.1 to sage9.0
 Reviewers set to 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 23 months ago by
 Commit changed from 887ca6a38642eb6eea4a8a7692a1b3139e65b112 to afa1d731ca5996c0cc0115bf144ca4b10db3e70d
Branch pushed to git repo; I updated commit sha1. New commits:
afa1d73  docs

comment:13 Changed 23 months ago by
 Status changed from needs_review to positive_review
comment:14 Changed 23 months ago by
 Branch changed from u/sbrandhorst/quadratic_form_from_invariants__ to afa1d731ca5996c0cc0115bf144ca4b10db3e70d
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
QuadraticFormFromInvariants first version