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: sage-8.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:

Status badges

Description (last modified by tscrim)

Write a method for the IntegralLattice class for generating the root lattices An, Dn, En, Euclidian lattice, U with standard basis.

Example:

IntegralLattice(["A",3])
IntegralLattice("U")
IntegralLattice(4)

Change History (25)

comment:1 Changed 4 years ago by pmenegat

  • Authors set to Paolo Menegatti
  • 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 pmenegat

  • Branch set to u/pmenegat/factory_function_for_famous_lattices

comment:3 Changed 4 years ago by pmenegat

  • Commit set to d4c282329fa0690bae48495e173ec17ad9403dc5
  • Status changed from new to needs_review

New commits:

d4c2823Added root lattices to `IntegralLattice` factory method

comment:4 Changed 4 years ago by tscrim

For the ADE latttices, I think you should follow the CartanType format of ['A', 2] or "A2". We should also think a little long-term 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 pmenegat

  • Status changed from needs_review to needs_work

comment:6 Changed 4 years ago by git

  • Commit changed from d4c282329fa0690bae48495e173ec17ad9403dc5 to 8aeb6d978083cf0ca34b2606d7684850d2ba5f7b

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

8aeb6d9Added compatibility with others ``CartanType`` format as ``['A',2]`` or ``""A2"``

comment:7 Changed 4 years ago by pmenegat

  • Status changed from needs_work to needs_review

comment:8 Changed 4 years ago by sbrandhorst

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

  • Commit changed from 8aeb6d978083cf0ca34b2606d7684850d2ba5f7b to 9337620c17f8b76a5cbc96e9c2333d90f65f44c4
  • Dependencies set to #23699

Last 10 new commits:

e5cecc5Polished docstrings
38091bbFixed indentation for docstrings at module level.
a7a9c39Lazy import IntegralLattice
6ca8126Formatting, minor doc changes, cleanup, and whitespace removal.
76987faSome trivial changes in the documentation.
12c8163Added b as an alias for _mul_.
c8ce05bAdded documentation for `b` and `inner_product`
4569a08Fixed doctest failures from changed printing.
8e983d0Merge branch 'public/lattices/torsion_quadratic_module_symmetric-23699' of git://trac.sagemath.org/sage into t/24150/factory_function_for_famous_lattices
9337620Use the new factory function in the documentation of the other methods and clean up.

comment:10 Changed 4 years ago by git

  • Commit changed from 9337620c17f8b76a5cbc96e9c2333d90f65f44c4 to e0057df4524a8d5b90750fcef6462f049abdf38f

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

e0057dfremove a superflous blank line

comment:11 Changed 4 years ago by git

  • Commit changed from e0057df4524a8d5b90750fcef6462f049abdf38f to 27084a9c57acaead4b590a7b36d5f006f47f1eda

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

27084a9Reducing lines to <80 characters

comment:12 Changed 4 years ago by git

  • Commit changed from 27084a9c57acaead4b590a7b36d5f006f47f1eda to 94757dd0ab91eb8d7953c3e6fbba3b3da2ed7e7b

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

94757ddFixed a typo. Doctests pass now.

comment:13 Changed 4 years ago by sbrandhorst

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

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 git

  • Commit changed from 94757dd0ab91eb8d7953c3e6fbba3b3da2ed7e7b to 63dc4854d84108d4f30e3d00248e7d98f5f731e2

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

63dc485a few extra spaces after a comma

comment:16 Changed 4 years ago by git

  • Commit changed from 63dc4854d84108d4f30e3d00248e7d98f5f731e2 to 220f941cb775798f49a857cfc3b130ade109107f

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

220f941Some more small docfixes

comment:17 Changed 4 years ago by sbrandhorst

The doc builds and looks fine.

comment:18 Changed 4 years ago by tscrim

  • Branch changed from u/sbrandhorst/factory_function_for_famous_lattices to public/quadratic_forms/lattice_factory-24150
  • 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:

d4cf39fDoing some code cleaning, improvements, and tweaks.

comment:19 Changed 4 years ago by pmenegat

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

  • Status changed from positive_review to needs_work

Doctests fail with #24096

comment:21 Changed 4 years ago by tscrim

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

  • Commit changed from d4cf39fe936cef854efb62e439d3cf34eedc7cfb to 32a6932670cee52f11a862d185ddbf34654adf21

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

325a26cMerge branch 'develop' of git://trac.sagemath.org/sage into t/24150/public/quadratic_forms/lattice_factory-24150
c670f54Use sage.structue.element.is_Matrix
c970ec5Deprecate sage.matrix.matrix
32a6932Merge branch 'u/jdemeyer/deprecate_sage_matrix_matrix' of git://trac.sagemath.org/sage into t/24150/public/quadratic_forms/lattice_factory-24150

comment:23 Changed 4 years ago by sbrandhorst

O.K. Let's see what patchbot has to say.

comment:24 Changed 4 years ago by tscrim

  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from needs_work to positive_review

comment:25 Changed 4 years ago by vbraun

  • Branch changed from public/quadratic_forms/lattice_factory-24150 to 32a6932670cee52f11a862d185ddbf34654adf21
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.