Opened 2 years ago

Closed 2 years ago

#23699 closed enhancement (fixed)

torsion_quadratic_module_symmetric

Reported by: sbrandhorst Owned by:
Priority: major Milestone: sage-8.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) 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 p-primary parts

Change History (62)

comment:1 Changed 2 years ago by sbrandhorst

  • Branch set to u/sbrandhorst/torsion_quadratic_module_symmetric

comment:2 Changed 2 years ago by amy

  • Branch changed from u/sbrandhorst/torsion_quadratic_module_symmetric to u/amy/torsion_quadratic_module_symmetric

comment:3 Changed 2 years ago by git

  • Commit set to e551d2c65153d69d8164d330152b5b3a409b62e9

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

e551d2cEdit formatting

comment:4 Changed 2 years ago by roed

  • Keywords sd91 added

comment:5 Changed 2 years ago by sbrandhorst

  • Dependencies set to 23915

comment:6 Changed 2 years ago by roed

  • Dependencies changed from 23915 to #23915

comment:7 Changed 2 years ago by roed

  • Branch changed from u/amy/torsion_quadratic_module_symmetric to u/roed/torsion_quadratic_module_symmetric

comment:8 Changed 2 years ago by roed

  • Commit changed from e551d2c65153d69d8164d330152b5b3a409b62e9 to 77d176c40006d1d632c363d622e0691b71ac14dd
  • Dependencies changed from #23915 to #23915, #23944

New commits:

e631647Initial version of Q/Z with no documentation
6955600Documented qmodnz.py and qmodnz_element.py
c02ff2aFixing category, adding UniqueRepresentation, tidying docstrings
b0ce12aMoving qmodz to additive_abelian
dc34258Fallout from the move to additive abelian
4ad5dbdRemove some blank lines
466f18aMerge branch 'qmodz' into t/23699/torsion_quadratic_module
20021c1Style changes
77d176cStyle changes, multiplication for inner product, change how generators are handled

comment:9 Changed 2 years ago by sbrandhorst

  • Branch changed from u/roed/torsion_quadratic_module_symmetric to u/sbrandhorst/torsion_quadratic_module_symmetric

comment:10 Changed 2 years ago by git

  • Commit changed from 77d176c40006d1d632c363d622e0691b71ac14dd to c9391a766efa9918ae7bc231e2536e65146b4758

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

dc87c98Merge branch 't/23703/free_quadratic_module_intersection_returns_wrong_result' into t/23699/torsion_quadratic_module_symmetric
3cf5ecfMerge branch 'develop' into t/23699/torsion_quadratic_module_symmetric
ce97a2eFixed _module_constructor and some documentation
c9391a7new implementation of orthogonal_submodule(self)

comment:11 Changed 2 years ago by sbrandhorst

  • Dependencies changed from #23915, #23944 to #23915, #23944,#23981

comment:12 Changed 2 years ago by git

  • Commit changed from c9391a766efa9918ae7bc231e2536e65146b4758 to fd4bb56fbee00044310867b3e8dd256a73ab3d99

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

fd4bb56more documentation

comment:13 Changed 2 years ago by git

  • Commit changed from fd4bb56fbee00044310867b3e8dd256a73ab3d99 to c8d77bb12f7f97d8062f8ae2e33ae135fd6b6fa4

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

bb54219Merge branch 't/23703/free_quadratic_module_intersection_returns_wrong_result' into t/23915/include_the_inner_product_matrix_in_module_comparison
57800c7Included the inner_product_matrix for comparison
b3fa051Moved doctest
0a0fbe9Adapted _eq_ for sage.categories.Homset and modifided code for pushout where != was used
e5d12f1Merge branch 't/23915/0a0fbe9a30f6fe4c0f1414f5710f1d1900572b5c' into t/23634/a_base_class_for_integral_quadratic_forms_seen_as_modules_with_a_bilinear_form_
d727d6cFixed orthogonal_complement()
06b8167Fixed 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.
707a74eclean up in __richcmp__ for free modules.
38a3a1fMerge branch 't/23634/a_base_class_for_integral_quadratic_forms_seen_as_modules_with_a_bilinear_form_' into t/23699/torsion_quadratic_module_symmetric
c8d77bbAdded a new attribute _value_module_qf and more doctests

comment:14 Changed 2 years ago by git

  • Commit changed from c8d77bb12f7f97d8062f8ae2e33ae135fd6b6fa4 to 96859fa9bbbf1029be559f50042426e0db2cb876

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

96859faMore doctests and sorted methods alphabetically

comment:15 Changed 2 years ago by sbrandhorst

  • Status changed from new to needs_review

comment:16 Changed 2 years ago by sbrandhorst

  • Dependencies changed from #23915, #23944,#23981 to #23915, #23944

comment:17 Changed 2 years ago by git

  • Commit changed from 96859fa9bbbf1029be559f50042426e0db2cb876 to b809ccb2ffa6da61fbd702a4d64f7f8a65ccc115

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

437bba1Merge branch 'develop' into t/23699/torsion_quadratic_module_symmetric
b809ccbAdded submodule_with_gens

comment:18 Changed 2 years ago by git

  • Commit changed from b809ccb2ffa6da61fbd702a4d64f7f8a65ccc115 to e0f3c700b7bbfc63afb4113bf4abb5c37dc41218

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

e0f3c70Fixed a bug in orthogonal_submodule to and allow generators as input

comment:19 Changed 2 years ago by git

  • Commit changed from e0f3c700b7bbfc63afb4113bf4abb5c37dc41218 to d2f5801904a3e2256df8d5515b2bc1c90019978d

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

d2f5801Cleaned up author history

comment:20 Changed 2 years ago by git

  • Commit changed from d2f5801904a3e2256df8d5515b2bc1c90019978d to 1c34ba312e64e39dfb0b7923f1426d6bf4efb2d0

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

1c34ba3Another bugfix

comment:21 Changed 2 years ago by git

  • Commit changed from 1c34ba312e64e39dfb0b7923f1426d6bf4efb2d0 to d498dd5ec5acedcc024629477fb1714089665268

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

658bc3dMerge branch 'develop' into t/23699/torsion_quadratic_module_symmetric
d498dd5Doctests pass now

comment:22 Changed 2 years ago by ursula

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 2 years ago by ursula

  • Keywords sd90 added

comment:24 Changed 2 years ago by ursula

The primary_part docstring has a typo: it says "Return the m-primary part of this torsion quadraticm module as a submodule", with an extra m on the end of 'quadratic'.

comment:25 Changed 2 years ago by git

  • Commit changed from d498dd5ec5acedcc024629477fb1714089665268 to 4e5642df988c953d6d46b8dcdacabb502abf07ef

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

4e5642dFixed documentation. Added it to the index, and added IntegralLattice TorsionQuadraticModule to the global namespace

comment:26 Changed 2 years ago by git

  • Commit changed from 4e5642df988c953d6d46b8dcdacabb502abf07ef to 69da8cda825ec75f0b8f0221db32cf8057adef13

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

69da8cdFall docfix

comment:27 Changed 2 years ago by ursula

The documentation builds, but the math mode doesn't display correctly (at least in my installation). All doctests pass.

comment:28 Changed 2 years ago by ursula

  • 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 2 years ago by ursula

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 2 years ago by ursula

Replacing `` with $$ in the documentation solves the problems with LaTeX display.

comment:31 follow-up: Changed 2 years ago by 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.

Last edited 2 years ago by sbrandhorst (previous) (diff)

comment:32 Changed 2 years ago by git

  • Commit changed from 69da8cda825ec75f0b8f0221db32cf8057adef13 to df648af6b4bb9f0d087c0588c2d16e6e10959540

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

df648afRemoved raw strings. Math renders now.

comment:33 Changed 2 years ago by git

  • Commit changed from df648af6b4bb9f0d087c0588c2d16e6e10959540 to a673fa1acdf213efba809b105adfa8c56d07ab26

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

a673fa1Doc polishing

comment:34 Changed 2 years ago by git

  • Commit changed from a673fa1acdf213efba809b105adfa8c56d07ab26 to ea6072a8e4c6dcc1b4fa4ef42b40df52bc9754a1

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

ea6072aMore docs...

comment:35 Changed 2 years ago by sbrandhorst

  • Status changed from needs_work to needs_review

comment:36 in reply to: ↑ 31 Changed 2 years ago by roed

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 2 years ago by git

  • Commit changed from ea6072a8e4c6dcc1b4fa4ef42b40df52bc9754a1 to 82a12933347f0dc0ed5a10d407cc67c1c1c59410

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

82a1293back to raw strings

comment:38 Changed 2 years ago by sbrandhorst

Indeed less backslashes look nicer.


New commits:

82a1293back to raw strings
Last edited 2 years ago by sbrandhorst (previous) (diff)

comment:39 Changed 2 years ago by tscrim

  • Status changed from needs_review to needs_work

Some questions:

  • Should we import TorsionQuadraticModule and IntegralLattice 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 in modules/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 the TestSuite(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/full-stop.
  • Many one-line descriptions are missing a period/full-stop.
  • gram_matrix_quadratic OUTPUT: is over indented.

comment:40 Changed 2 years ago by git

  • Commit changed from 82a12933347f0dc0ed5a10d407cc67c1c1c59410 to e5cecc58270b20f8598c7a13e678bf06e1015031

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

e5cecc5Polished docstrings

comment:41 Changed 2 years ago by sbrandhorst

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 ADE-lattices, Leech, ... or an interface with the lmfdb.

comment:42 Changed 2 years ago by sbrandhorst

  • Status changed from needs_work to needs_review

comment:43 follow-up: Changed 2 years ago by ursula

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 2 years ago by sbrandhorst

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 2 years ago by tscrim

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 module-level docstring are over-indented.

Ursula, are you going to be handling the rest of the review, or at least the mathematical side?

comment:46 Changed 2 years ago by ursula

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 2 years ago by tscrim

If the math is good, then I can do the rest of the review.

comment:48 Changed 2 years ago by git

  • Commit changed from e5cecc58270b20f8598c7a13e678bf06e1015031 to 38091bb39a486266e418c7e4153771d1667c8dac

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

38091bbFixed indentation for docstrings at module level.

comment:49 Changed 2 years ago by sbrandhorst

*ping* :-)

comment:50 Changed 2 years ago by ursula

Confirmed documentation now displays math correctly!

comment:51 Changed 2 years ago by git

  • Commit changed from 38091bb39a486266e418c7e4153771d1667c8dac to a7a9c3984ea99a152a4262f8b6baf19c67b09566

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

a7a9c39Lazy import IntegralLattice

comment:52 Changed 2 years ago by tscrim

  • Branch changed from u/sbrandhorst/torsion_quadratic_module_symmetric to public/lattices/torsion_quadratic_module_symmetric-23699
  • 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 top-level 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:

6ca8126Formatting, minor doc changes, cleanup, and whitespace removal.

comment:53 Changed 2 years ago by git

  • Commit changed from 6ca81266119627ef262b285d5cd639369e299945 to 76987fa27a1cde484c985914d7f69d311de5e47f

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

76987faSome trivial changes in the documentation.

comment:54 Changed 2 years ago by git

  • Commit changed from 76987fa27a1cde484c985914d7f69d311de5e47f to 12c81635139a8fe236e538f20a50332b242237d4

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

12c8163Added b as an alias for _mul_.

comment:55 Changed 2 years ago by git

  • Commit changed from 12c81635139a8fe236e538f20a50332b242237d4 to c8ce05bd73c8cd001b39ecb76d1e335bc14dbda3

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

c8ce05bAdded documentation for `b` and `inner_product`

comment:56 follow-up: Changed 2 years ago by sbrandhorst

  • Authors set to Simon Brandhorst
  • 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 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Replying to sbrandhorst:

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.

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 2 years ago by git

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

4569a08Fixed doctest failures from changed printing.

comment:59 Changed 2 years ago by sbrandhorst

Let us see what the patchbot says now.

comment:60 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Whoops, forgot about those. Thanks.

comment:61 Changed 2 years ago by sbrandhorst

There are some patchbot failures. However, they look quite unrelated.

comment:62 Changed 2 years ago by vbraun

  • Branch changed from public/lattices/torsion_quadratic_module_symmetric-23699 to 4569a08e868d30dacaa6f51cb4ff150ce9b1be96
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.