Opened 4 years ago
Closed 4 years ago
#24150 closed enhancement (fixed)
Factory function for famous lattices
Reported by:  pmenegat  Owned by:  pmenegat 

Priority:  major  Milestone:  sage8.2 
Component:  quadratic forms  Keywords:  
Cc:  Merged in:  
Authors:  Paolo Menegatti  Reviewers:  Simon Brandhorst, Travis Scrimshaw, Paolo Menegatti 
Report Upstream:  N/A  Work issues:  
Branch:  32a6932 (Commits, GitHub, GitLab)  Commit:  32a6932670cee52f11a862d185ddbf34654adf21 
Dependencies:  #23699, #24096  Stopgaps: 
Description (last modified by )
Write a method for the IntegralLattice
class for generating the root lattices A_{n}, D_{n}, E_{n}, Euclidian lattice, U with standard basis.
Example:
IntegralLattice(["A",3]) IntegralLattice("U") IntegralLattice(4)
Change History (25)
comment:1 Changed 4 years ago by
 Component changed from PLEASE CHANGE to quadratic forms
 Owner changed from (none) to pmenegat
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 4 years ago by
 Branch set to u/pmenegat/factory_function_for_famous_lattices
comment:3 Changed 4 years ago by
 Commit set to d4c282329fa0690bae48495e173ec17ad9403dc5
 Status changed from new to needs_review
comment:4 Changed 4 years ago by
For the ADE latttices, I think you should follow the CartanType
format of ['A', 2]
or "A2"
. We should also think a little longterm about how the root/weight lattice code in src/combinat/root_system/
will interact with the IntegralLattice
code.
comment:5 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:6 Changed 4 years ago by
 Commit changed from d4c282329fa0690bae48495e173ec17ad9403dc5 to 8aeb6d978083cf0ca34b2606d7684850d2ba5f7b
Branch pushed to git repo; I updated commit sha1. New commits:
8aeb6d9  Added compatibility with others ``CartanType`` format as ``['A',2]`` or ``""A2"``

comment:7 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:8 Changed 4 years ago by
 Branch changed from u/pmenegat/factory_function_for_famous_lattices to u/sbrandhorst/factory_function_for_famous_lattices
comment:9 Changed 4 years ago by
 Commit changed from 8aeb6d978083cf0ca34b2606d7684850d2ba5f7b to 9337620c17f8b76a5cbc96e9c2333d90f65f44c4
 Dependencies set to #23699
Last 10 new commits:
e5cecc5  Polished docstrings

38091bb  Fixed indentation for docstrings at module level.

a7a9c39  Lazy import IntegralLattice

6ca8126  Formatting, minor doc changes, cleanup, and whitespace removal.

76987fa  Some trivial changes in the documentation.

12c8163  Added b as an alias for _mul_.

c8ce05b  Added documentation for `b` and `inner_product`

4569a08  Fixed doctest failures from changed printing.

8e983d0  Merge branch 'public/lattices/torsion_quadratic_module_symmetric23699' of git://trac.sagemath.org/sage into t/24150/factory_function_for_famous_lattices

9337620  Use the new factory function in the documentation of the other methods and clean up.

comment:10 Changed 4 years ago by
 Commit changed from 9337620c17f8b76a5cbc96e9c2333d90f65f44c4 to e0057df4524a8d5b90750fcef6462f049abdf38f
Branch pushed to git repo; I updated commit sha1. New commits:
e0057df  remove a superflous blank line

comment:11 Changed 4 years ago by
 Commit changed from e0057df4524a8d5b90750fcef6462f049abdf38f to 27084a9c57acaead4b590a7b36d5f006f47f1eda
Branch pushed to git repo; I updated commit sha1. New commits:
27084a9  Reducing lines to <80 characters

comment:12 Changed 4 years ago by
 Commit changed from 27084a9c57acaead4b590a7b36d5f006f47f1eda to 94757dd0ab91eb8d7953c3e6fbba3b3da2ed7e7b
Branch pushed to git repo; I updated commit sha1. New commits:
94757dd  Fixed a typo. Doctests pass now.

comment:13 Changed 4 years ago by
 Reviewers set to Simon Brandhorst
I did some trivial modifications of the documentation. Positive review if you are happy with my changes.
comment:14 Changed 4 years ago by
The reason to merge in #23699 is that it adds IntegralLattice
to the global namespace and the documentation. Thus we can clean up the docstrings with this ticket.
comment:15 Changed 4 years ago by
 Commit changed from 94757dd0ab91eb8d7953c3e6fbba3b3da2ed7e7b to 63dc4854d84108d4f30e3d00248e7d98f5f731e2
Branch pushed to git repo; I updated commit sha1. New commits:
63dc485  a few extra spaces after a comma

comment:16 Changed 4 years ago by
 Commit changed from 63dc4854d84108d4f30e3d00248e7d98f5f731e2 to 220f941cb775798f49a857cfc3b130ade109107f
Branch pushed to git repo; I updated commit sha1. New commits:
220f941  Some more small docfixes

comment:17 Changed 4 years ago by
The doc builds and looks fine.
comment:18 Changed 4 years ago by
 Branch changed from u/sbrandhorst/factory_function_for_famous_lattices to public/quadratic_forms/lattice_factory24150
 Commit changed from 220f941cb775798f49a857cfc3b130ade109107f to d4cf39fe936cef854efb62e439d3cf34eedc7cfb
 Description modified (diff)
 Reviewers changed from Simon Brandhorst to Simon Brandhorst, Travis Scrimshaw
I got rid of the input IntegralLattice("A",3)
because you made the code unnecessarily complicated by supporting that, where it is more natural for the input to be something that is recognizable by CartanType
/CartanMatrix
/etc. Moreover, the code previously had a bug:
sage: H5 = Matrix(ZZ, 2, [2,1,1,2]) sage: IntegralLattice(H5, Matrix([1,1])) Lattice of degree 2 and rank 2 over Integer Ring Basis matrix: [1 0] [0 1] Inner product matrix: [ 2 1] [ 1 2] sage: IntegralLattice(H5, basis=Matrix([1,1])) Lattice of degree 2 and rank 1 over Integer Ring Basis matrix: [1 1] Inner product matrix: [ 2 1] [ 1 2]
whereas you claimed in the doc that they were the same. So if my changes look good (and this example is now correct and valid), then positive review.
New commits:
d4cf39f  Doing some code cleaning, improvements, and tweaks.

comment:19 Changed 4 years ago by
 Reviewers changed from Simon Brandhorst, Travis Scrimshaw to Simon Brandhorst, Travis Scrimshaw, Paolo Menegatti
 Status changed from needs_review to positive_review
comment:20 Changed 4 years ago by
 Status changed from positive_review to needs_work
Doctests fail with #24096
comment:21 Changed 4 years ago by
 Dependencies changed from #23699 to #23699, #24096
 Description modified (diff)
You (should) just need to make this change:
from sage.matrix.matrix import is_Matrix +from sage.structure.element import is_Matrix
comment:22 Changed 4 years ago by
 Commit changed from d4cf39fe936cef854efb62e439d3cf34eedc7cfb to 32a6932670cee52f11a862d185ddbf34654adf21
Branch pushed to git repo; I updated commit sha1. New commits:
325a26c  Merge branch 'develop' of git://trac.sagemath.org/sage into t/24150/public/quadratic_forms/lattice_factory24150

c670f54  Use sage.structue.element.is_Matrix

c970ec5  Deprecate sage.matrix.matrix

32a6932  Merge branch 'u/jdemeyer/deprecate_sage_matrix_matrix' of git://trac.sagemath.org/sage into t/24150/public/quadratic_forms/lattice_factory24150

comment:23 Changed 4 years ago by
O.K. Let's see what patchbot has to say.
comment:24 Changed 4 years ago by
 Milestone changed from sage8.1 to sage8.2
 Status changed from needs_work to positive_review
comment:25 Changed 4 years ago by
 Branch changed from public/quadratic_forms/lattice_factory24150 to 32a6932670cee52f11a862d185ddbf34654adf21
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Added root lattices to `IntegralLattice` factory method