Opened 5 years ago
Closed 5 years ago
#23699 closed enhancement (fixed)
torsion_quadratic_module_symmetric
Reported by:  sbrandhorst  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  linear algebra  Keywords:  sd91, sd90 
Cc:  Merged in:  
Authors:  Simon Brandhorst  Reviewers:  Travis Scrimshaw, Ursula Whitcher 
Report Upstream:  N/A  Work issues:  
Branch:  4569a08 (Commits, GitHub, GitLab)  Commit:  4569a08e868d30dacaa6f51cb4ff150ce9b1be96 
Dependencies:  #23915, #23944  Stopgaps: 
Description
We want to model the following: Let A be a finite abelian group. A (symmetric) torsion bilinear form is b: A x A > QQ/ZZ.
A torsion quadratic form is q: A > QQ/2ZZ such that q(na)=n*n*q(a) a for a in A.
This class should consist of:
 an Abelian group with a distinguished basis
 an "inner product matrix" defining the bilinear/quadratic form.
 Elements should have methods to compute q and b
It should behave much like sage/modules/free_quadratic_module
methods should include:
 create a free quadratic submodule from generators
 orthogonal complements
 a canonical form
 isomorphisms
 brown invariants
 the orthogonal decompositions into its pprimary parts
Change History (62)
comment:1 Changed 5 years ago by
 Branch set to u/sbrandhorst/torsion_quadratic_module_symmetric
comment:2 Changed 5 years ago by
 Branch changed from u/sbrandhorst/torsion_quadratic_module_symmetric to u/amy/torsion_quadratic_module_symmetric
comment:3 Changed 5 years ago by
 Commit set to e551d2c65153d69d8164d330152b5b3a409b62e9
comment:4 Changed 5 years ago by
 Keywords sd91 added
comment:5 Changed 5 years ago by
 Dependencies set to 23915
comment:6 Changed 5 years ago by
 Dependencies changed from 23915 to #23915
comment:7 Changed 5 years ago by
 Branch changed from u/amy/torsion_quadratic_module_symmetric to u/roed/torsion_quadratic_module_symmetric
comment:8 Changed 5 years ago by
 Commit changed from e551d2c65153d69d8164d330152b5b3a409b62e9 to 77d176c40006d1d632c363d622e0691b71ac14dd
 Dependencies changed from #23915 to #23915, #23944
New commits:
e631647  Initial version of Q/Z with no documentation

6955600  Documented qmodnz.py and qmodnz_element.py

c02ff2a  Fixing category, adding UniqueRepresentation, tidying docstrings

b0ce12a  Moving qmodz to additive_abelian

dc34258  Fallout from the move to additive abelian

4ad5dbd  Remove some blank lines

466f18a  Merge branch 'qmodz' into t/23699/torsion_quadratic_module

20021c1  Style changes

77d176c  Style changes, multiplication for inner product, change how generators are handled

comment:9 Changed 5 years ago by
 Branch changed from u/roed/torsion_quadratic_module_symmetric to u/sbrandhorst/torsion_quadratic_module_symmetric
comment:10 Changed 5 years ago by
 Commit changed from 77d176c40006d1d632c363d622e0691b71ac14dd to c9391a766efa9918ae7bc231e2536e65146b4758
Branch pushed to git repo; I updated commit sha1. New commits:
dc87c98  Merge branch 't/23703/free_quadratic_module_intersection_returns_wrong_result' into t/23699/torsion_quadratic_module_symmetric

3cf5ecf  Merge branch 'develop' into t/23699/torsion_quadratic_module_symmetric

ce97a2e  Fixed _module_constructor and some documentation

c9391a7  new implementation of orthogonal_submodule(self)

comment:11 Changed 5 years ago by
 Dependencies changed from #23915, #23944 to #23915, #23944,#23981
comment:12 Changed 5 years ago by
 Commit changed from c9391a766efa9918ae7bc231e2536e65146b4758 to fd4bb56fbee00044310867b3e8dd256a73ab3d99
Branch pushed to git repo; I updated commit sha1. New commits:
fd4bb56  more documentation

comment:13 Changed 5 years ago by
 Commit changed from fd4bb56fbee00044310867b3e8dd256a73ab3d99 to c8d77bb12f7f97d8062f8ae2e33ae135fd6b6fa4
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
bb54219  Merge branch 't/23703/free_quadratic_module_intersection_returns_wrong_result' into t/23915/include_the_inner_product_matrix_in_module_comparison

57800c7  Included the inner_product_matrix for comparison

b3fa051  Moved doctest

0a0fbe9  Adapted _eq_ for sage.categories.Homset and modifided code for pushout where != was used

e5d12f1  Merge branch 't/23915/0a0fbe9a30f6fe4c0f1414f5710f1d1900572b5c' into t/23634/a_base_class_for_integral_quadratic_forms_seen_as_modules_with_a_bilinear_form_

d727d6c  Fixed orthogonal_complement()

06b8167  Fixed the loads(dumps(x)) issue by fixing comparison for free modules. The change only kicks in if the unique parent assumption is violated. This seems to happen in the loads dumps process.

707a74e  clean up in __richcmp__ for free modules.

38a3a1f  Merge branch 't/23634/a_base_class_for_integral_quadratic_forms_seen_as_modules_with_a_bilinear_form_' into t/23699/torsion_quadratic_module_symmetric

c8d77bb  Added a new attribute _value_module_qf and more doctests

comment:14 Changed 5 years ago by
 Commit changed from c8d77bb12f7f97d8062f8ae2e33ae135fd6b6fa4 to 96859fa9bbbf1029be559f50042426e0db2cb876
Branch pushed to git repo; I updated commit sha1. New commits:
96859fa  More doctests and sorted methods alphabetically

comment:15 Changed 5 years ago by
 Status changed from new to needs_review
comment:16 Changed 5 years ago by
 Dependencies changed from #23915, #23944,#23981 to #23915, #23944
comment:17 Changed 5 years ago by
 Commit changed from 96859fa9bbbf1029be559f50042426e0db2cb876 to b809ccb2ffa6da61fbd702a4d64f7f8a65ccc115
comment:18 Changed 5 years ago by
 Commit changed from b809ccb2ffa6da61fbd702a4d64f7f8a65ccc115 to e0f3c700b7bbfc63afb4113bf4abb5c37dc41218
Branch pushed to git repo; I updated commit sha1. New commits:
e0f3c70  Fixed a bug in orthogonal_submodule to and allow generators as input

comment:19 Changed 5 years ago by
 Commit changed from e0f3c700b7bbfc63afb4113bf4abb5c37dc41218 to d2f5801904a3e2256df8d5515b2bc1c90019978d
Branch pushed to git repo; I updated commit sha1. New commits:
d2f5801  Cleaned up author history

comment:20 Changed 5 years ago by
 Commit changed from d2f5801904a3e2256df8d5515b2bc1c90019978d to 1c34ba312e64e39dfb0b7923f1426d6bf4efb2d0
Branch pushed to git repo; I updated commit sha1. New commits:
1c34ba3  Another bugfix

comment:21 Changed 5 years ago by
 Commit changed from 1c34ba312e64e39dfb0b7923f1426d6bf4efb2d0 to d498dd5ec5acedcc024629477fb1714089665268
comment:22 Changed 5 years ago by
Why do all of the examples start with an import command? It seems like this is basic functionality that should be available within Sage without extra work.
Minor note: the first documentation line of TorsionQuadraticModule
doesn't wrap properly.
comment:23 Changed 5 years ago by
 Keywords sd90 added
comment:24 Changed 5 years ago by
The primary_part docstring has a typo: it says "Return the mprimary part of this torsion quadraticm module as a submodule", with an extra m on the end of 'quadratic'.
comment:25 Changed 5 years ago by
 Commit changed from d498dd5ec5acedcc024629477fb1714089665268 to 4e5642df988c953d6d46b8dcdacabb502abf07ef
Branch pushed to git repo; I updated commit sha1. New commits:
4e5642d  Fixed documentation. Added it to the index, and added IntegralLattice TorsionQuadraticModule to the global namespace

comment:26 Changed 5 years ago by
 Commit changed from 4e5642df988c953d6d46b8dcdacabb502abf07ef to 69da8cda825ec75f0b8f0221db32cf8057adef13
Branch pushed to git repo; I updated commit sha1. New commits:
69da8cd  Fall docfix

comment:27 Changed 5 years ago by
The documentation builds, but the math mode doesn't display correctly (at least in my installation). All doctests pass.
comment:28 Changed 5 years ago by
 Status changed from needs_review to needs_work
Many of the examples use T.0 or T.1. Please use T.gen(0), etc. instead: this makes it easier to look up the intended meaning of the code.
comment:29 Changed 5 years ago by
The example for gram_matrix_quadratic constructs a matrix called D4_gram directly. The command DynkinDiagram('D',4).cartan_matrix()
will create the same matrix. Would it be clearer to do so?
comment:30 Changed 5 years ago by
Replacing ``
with $$ in the documentation solves the problems with LaTeX display.
comment:31 followup: ↓ 36 Changed 5 years ago by
Actually the confusion was between strings and raw strings. Some were raw and some not. I removed all raw strings. For me it works fine with chrome. With Firefox I get some math processing errors. But I guess that is just because the browser is outdated.
comment:32 Changed 5 years ago by
 Commit changed from 69da8cda825ec75f0b8f0221db32cf8057adef13 to df648af6b4bb9f0d087c0588c2d16e6e10959540
Branch pushed to git repo; I updated commit sha1. New commits:
df648af  Removed raw strings. Math renders now.

comment:33 Changed 5 years ago by
 Commit changed from df648af6b4bb9f0d087c0588c2d16e6e10959540 to a673fa1acdf213efba809b105adfa8c56d07ab26
Branch pushed to git repo; I updated commit sha1. New commits:
a673fa1  Doc polishing

comment:34 Changed 5 years ago by
 Commit changed from a673fa1acdf213efba809b105adfa8c56d07ab26 to ea6072a8e4c6dcc1b4fa4ef42b40df52bc9754a1
Branch pushed to git repo; I updated commit sha1. New commits:
ea6072a  More docs...

comment:35 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:36 in reply to: ↑ 31 Changed 5 years ago by
Replying to sbrandhorst:
Actually the confusion was between strings and raw strings. Some were raw and some not. I removed all raw strings. For me it works fine with chrome. With Firefox I get some math processing errors. But I guess that is just because the browser is outdated.
Why did you remove all the raw strings? I would have gone the other way and made all docstrings raw strings....
Also, you shouldn't use $ in documentation, but single backticks for mathmode (or .. mathmode::
for display mode).
comment:37 Changed 5 years ago by
 Commit changed from ea6072a8e4c6dcc1b4fa4ef42b40df52bc9754a1 to 82a12933347f0dc0ed5a10d407cc67c1c1c59410
Branch pushed to git repo; I updated commit sha1. New commits:
82a1293  back to raw strings

comment:38 Changed 5 years ago by
comment:39 Changed 5 years ago by
 Status changed from needs_review to needs_work
Some questions:
 Should we import
TorsionQuadraticModule
andIntegralLattice
into the global namespace? I feel like there are more natural constructions for these modules, say as a quotient object and by using vector space constructions. That way we do not pollute the global namespace. There probably is a stronger case not to import the former, but it is good to avoid both.  Is
free_quadratic_module_integer_symmetric.py
included in the documentation? I don't see it inmodules/index.rst
.
Some comments:
 You should do tests with
TestSuite(foo).run()
.  Your
TorsionQuadraticModuleElement.__init__
docstring closing"""
is not correctly indented.  It should be
``self``
in docstrings since it is Python code.  Try to keep lines <79 characters, especially in documentation.
 Not every method has tests (
__init__
is a good place for theTestSuite(foo).run()
and is where I always put them).  Change this and similar for Python3 and general Python error string conventions
ValueError, "The modulus must divide (V,W)." +ValueError("the modulus must divide (V,W)")
INPUT:
items do not end with a period/fullstop. Many oneline descriptions are missing a period/fullstop.
gram_matrix_quadratic
OUTPUT:
is over indented.
comment:40 Changed 5 years ago by
 Commit changed from 82a12933347f0dc0ed5a10d407cc67c1c1c59410 to e5cecc58270b20f8598c7a13e678bf06e1015031
Branch pushed to git repo; I updated commit sha1. New commits:
e5cecc5  Polished docstrings

comment:41 Changed 5 years ago by
O.K. the use case that I have in mind is to create a TorsionQuadraticModule
from an
IntegralLattice
. So there is indeed no use to pollute the global namespace.
You are right, one could check if a free_quadratic_module
is an IntegralLattice
in the FreeQuadraticModule
factory function. Do we want that?
I any case I want an IntegralLattice
factory function in the global namespace.
At the moment it has little functionality, but later I plan to extend its functionality to create
lattices from keywords, like the ADElattices, Leech, ... or an interface with the lmfdb.
comment:42 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:43 followup: ↓ 44 Changed 5 years ago by
As someone who works in an area of geometry that uses these objects, I absolutely want IntegralLattice
as part of the global name space. I also want the ability to work directly with the associated discriminant groups, and ask questions about whether one is isomorphic to another, in the same genus, etc. There are some classifications of possible discriminant groups which mean being able to access TorsionQuadraticModule
directly might be helpful.
Creating ADE lattices should be very quick, since as I note above we already have the appropriate matrices in the DynkinDiagram
code.
comment:44 in reply to: ↑ 43 Changed 5 years ago by
Replying to ursula:
Creating ADE lattices should be very quick, since as I note above we already have the appropriate matrices in the
DynkinDiagram
code.
Let's make the factory functions a separate ticket.
comment:45 Changed 5 years ago by
Fair enough. This is not my area of expertise, so if you think it is useful, then I have no objections other than it should be a lazy_import
. My suggestion stemmed from thinking such a construction would be similar to this:
sage: V = (ZZ^3).span([[1/2,5,2],[3/7,2,1],[1/3,1/5,2]])
Also, one other minor doc point, in free_quadratic_module_integer_symmetric.py
, the examples in the modulelevel docstring are overindented.
Ursula, are you going to be handling the rest of the review, or at least the mathematical side?
comment:46 Changed 5 years ago by
I'm definitely looking at the math but don't have a lot of reviewing time outside Sage Days, so any help is appreciated.
comment:47 Changed 5 years ago by
If the math is good, then I can do the rest of the review.
comment:48 Changed 5 years ago by
 Commit changed from e5cecc58270b20f8598c7a13e678bf06e1015031 to 38091bb39a486266e418c7e4153771d1667c8dac
Branch pushed to git repo; I updated commit sha1. New commits:
38091bb  Fixed indentation for docstrings at module level.

comment:49 Changed 5 years ago by
*ping* :)
comment:50 Changed 5 years ago by
Confirmed documentation now displays math correctly!
comment:51 Changed 5 years ago by
 Commit changed from 38091bb39a486266e418c7e4153771d1667c8dac to a7a9c3984ea99a152a4262f8b6baf19c67b09566
Branch pushed to git repo; I updated commit sha1. New commits:
a7a9c39  Lazy import IntegralLattice

comment:52 Changed 5 years ago by
 Branch changed from u/sbrandhorst/torsion_quadratic_module_symmetric to public/lattices/torsion_quadratic_module_symmetric23699
 Commit changed from a7a9c3984ea99a152a4262f8b6baf19c67b09566 to 6ca81266119627ef262b285d5cd639369e299945
 Reviewers set to Travis Scrimshaw
I went through and made a lot of mostly minor changes to keep line lengths short, code and doc clean and as consistent as is possible in Sage, and removing trailing whitespace. It would be nice to have a natural toplevel way to construct a TorsionQuadraticModule
that is not adding to the global namespace. That is to say, we should be able to do V / W
for appropriate objects V
and W
and/or something like V.torsion_quotient(W)
(I am sure that is not a good name, please suggest alternatives). I just am not sure enough in the math to say what V
and W
should be. However, if that is something that could wait for another ticket, then you cab set a positive review if my changes look good and you add your name to the authors (and @ursula to the reviewers).
New commits:
6ca8126  Formatting, minor doc changes, cleanup, and whitespace removal.

comment:53 Changed 5 years ago by
 Commit changed from 6ca81266119627ef262b285d5cd639369e299945 to 76987fa27a1cde484c985914d7f69d311de5e47f
Branch pushed to git repo; I updated commit sha1. New commits:
76987fa  Some trivial changes in the documentation.

comment:54 Changed 5 years ago by
 Commit changed from 76987fa27a1cde484c985914d7f69d311de5e47f to 12c81635139a8fe236e538f20a50332b242237d4
Branch pushed to git repo; I updated commit sha1. New commits:
12c8163  Added b as an alias for _mul_.

comment:55 Changed 5 years ago by
 Commit changed from 12c81635139a8fe236e538f20a50332b242237d4 to c8ce05bd73c8cd001b39ecb76d1e335bc14dbda3
Branch pushed to git repo; I updated commit sha1. New commits:
c8ce05b  Added documentation for `b` and `inner_product`

comment:56 followup: ↓ 57 Changed 5 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Ursula Whitcher
We could enable V/W
. But since that does not work for all V, W
it feels odd.
I would delegate such decisions to a different ticket if the need arises.
Thank you for the review. I am happy with your changes and did some trivial additions. Positive review if you are happy with my changes.
comment:57 in reply to: ↑ 56 Changed 5 years ago by
 Status changed from needs_review to positive_review
Replying to sbrandhorst:
We could enable
V/W
. But since that does not work for allV, W
it feels odd. I would delegate such decisions to a different ticket if the need arises.
It doesn't feel that odd to me. That is what generic code and NotImplementedError
's are for. However, a later ticket it will be.
Thank you for the review. I am happy with your changes and did some trivial additions. Positive review if you are happy with my changes.
Thank you.
comment:58 Changed 5 years ago by
 Commit changed from c8ce05bd73c8cd001b39ecb76d1e335bc14dbda3 to 4569a08e868d30dacaa6f51cb4ff150ce9b1be96
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
4569a08  Fixed doctest failures from changed printing.

comment:59 Changed 5 years ago by
Let us see what the patchbot says now.
comment:60 Changed 5 years ago by
 Status changed from needs_review to positive_review
Whoops, forgot about those. Thanks.
comment:61 Changed 5 years ago by
There are some patchbot failures. However, they look quite unrelated.
comment:62 Changed 5 years ago by
 Branch changed from public/lattices/torsion_quadratic_module_symmetric23699 to 4569a08e868d30dacaa6f51cb4ff150ce9b1be96
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Edit formatting