Opened 2 years ago

Closed 3 months ago

#24108 closed enhancement (fixed)

quadratic_form_from_invariants()

Reported by: annahaensch Owned by:
Priority: major Milestone: sage-9.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) 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 2 years ago by annahaensch

  • Authors changed from mroy, sbrudzin, jduque to annahaensch, mroy, sbrudzin, jduque

comment:2 Changed 5 months ago by sbrandhorst

  • Branch set to u/sbrandhorst/quadratic_form_from_invariants__

comment:3 Changed 5 months ago by sbrandhorst

  • Authors changed from annahaensch, mroy, sbrudzin, jduque to Simon Brandhorst
  • Commit set to 09526929769530789b71d41c00ba54b3674498cc
  • Status changed from new to needs_review

New commits:

0952692QuadraticFormFromInvariants first version

comment:4 follow-up: Changed 4 months ago by tscrim

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 ; follow-up: Changed 4 months ago by 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?

comment:6 in reply to: ↑ 5 Changed 4 months ago by tscrim

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 follow-up: Changed 4 months ago by 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?

comment:8 in reply to: ↑ 7 Changed 4 months ago by tscrim

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 4 months ago by git

  • Commit changed from 09526929769530789b71d41c00ba54b3674498cc to 887ca6a38642eb6eea4a8a7692a1b3139e65b112

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

887ca6asmall improvements for quadratic_form_from_invariants

comment:10 Changed 4 months ago by sbrandhorst

I did it as quadratic_form_from_invariants.


New commits:

887ca6asmall improvements for quadratic_form_from_invariants

comment:11 Changed 4 months ago by tscrim

  • Milestone changed from sage-8.1 to sage-9.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 4 months ago by git

  • Commit changed from 887ca6a38642eb6eea4a8a7692a1b3139e65b112 to afa1d731ca5996c0cc0115bf144ca4b10db3e70d

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

afa1d73docs

comment:13 Changed 4 months ago by sbrandhorst

  • Status changed from needs_review to positive_review

comment:14 Changed 3 months ago by vbraun

  • Branch changed from u/sbrandhorst/quadratic_form_from_invariants__ to afa1d731ca5996c0cc0115bf144ca4b10db3e70d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.