Opened 6 years ago

Closed 3 years ago

#16820 closed enhancement (fixed)

Implement ABCs and categories for Lie algebras and finite dimensional Lie algebras given by structure coefficients

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-8.0
Component: algebra Keywords: lie algebras, days64, sd67, days74, days79
Cc: Merged in:
Authors: Travis Scrimshaw Reviewers: Darij Grinberg, Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 98c488d (Commits) Commit: 98c488db05e40d8fd099673e8f8274febc2a8657
Dependencies: Stopgaps:

Description (last modified by tscrim)

Part of #14901. This implements the basic hierarchy of base classes, the categories for Lie algebras, some standard Lie algebras, and finite-dimensional Lie algebras given by structure coefficients.

Change History (146)

comment:1 Changed 5 years ago by tscrim

  • Branch set to public/lie_algebras/fd_structure_coeff-16820
  • Commit set to c2a3a982e419c0839f1fba68bf71b1014f243547
  • Status changed from new to needs_review

Last 10 new commits:

b35c8d3Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
44a17b9Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
59c0a96Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
48ec188Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
cbfbb87Merge branch 'public/categories/lie_algebras-16819' of trac.sagemath.org:sage into public/categories/lie_algebras-16819
ed93f29Created category for subalgebras of FD Lie algebras with basis.
1a17ff9Merge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820
b302cc1Added free_module abstract method.
96083bfMerge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820
c2a3a98Many changes, fixes, and adding full doctest coverage.

comment:2 Changed 5 years ago by git

  • Commit changed from c2a3a982e419c0839f1fba68bf71b1014f243547 to e5c1703bc6c958566a5f2a91d909e45b52b3c2b0

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

c1f3eb3Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
d0ddb8cMerge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
7424e75Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
e5c1703Fixed typos with sl2 construction.

comment:3 Changed 5 years ago by git

  • Commit changed from e5c1703bc6c958566a5f2a91d909e45b52b3c2b0 to 87bba39ede0cfea37bb5e92b81aa851a2420863e

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

45db323Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
036995fCleaning up/Fixing some logic, in particular for FromAssociative.
a135fb3Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
b4b89daFixing errors introduced by logic changes.
e9587d7Added a custom lift morphism for Lie alg from assoc.
87bba39Fixing the last issues from the changes.

comment:4 Changed 5 years ago by git

  • Commit changed from 87bba39ede0cfea37bb5e92b81aa851a2420863e to ece3cd9d8ae826f28b82935d638de1f5df9d90a9

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

65c1f54Removed the LiftMorphism.section map.
becabaaMerge branch 'public/categories/lie_algebras-16819' of trac.sagemath.org:sage into public/categories/lie_algebras-16819
980545eFixed the doc.
8a6950aMerge branch 'public/categories/lie_algebras-16819' of git://trac.sagemath.org/sage into lie1
9b08311a few questions to boot
5a0bc7dAnswering some questions.
144a5a5Small fix for the doc.
198f8a8Added new categories to documentation.
aeb6075Merge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820
ece3cd9Added files to the documentation.

comment:5 Changed 5 years ago by git

  • Commit changed from ece3cd9d8ae826f28b82935d638de1f5df9d90a9 to 845e5aa14b7c97edeb04859fc92ebc63a78be1cb

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

5197c7cMerge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
845e5aaSome last tweaks and getting full coverage.

comment:6 Changed 5 years ago by tscrim

  • Keywords lie algebras days64 added
  • Milestone changed from sage-6.4 to sage-6.6

comment:7 Changed 5 years ago by git

  • Commit changed from 845e5aa14b7c97edeb04859fc92ebc63a78be1cb to 12074e86295e2848a520c5962678c1c9e75b7505

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

896028aA y for an x.
2d2a54dMerge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820
6ca7acaFixing some parts of _construct_UEA.
d35b756Adding a default _basis_ordering and _dense_free_module
0e33276Merge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820
e95d67cMore fixes to fin-dim w/ basis category.
c800d6cMerge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820
a491bb0Some more cleanup and fixes.
aabf540Fixing coefficient division.
12074e8Merge branch 'public/categories/lie_algebras-16819' into public/lie_algebras/fd_structure_coeff-16820

comment:8 Changed 5 years ago by git

  • Commit changed from 12074e86295e2848a520c5962678c1c9e75b7505 to 9830d63d19d1c4ec432a83ab8c21685ec7aae802

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

464c131another appearance of free_module
6608a18.module and .to_vector: I still don't get them
106d75dMerge branch 'public/categories/lie_algebras-16819' of git://trac.sagemath.org/sage into lie1
6201720does this make sense?
ddc77b1Merge branch 'public/categories/lie_algebras-16819' of git://trac.sagemath.org/sage into lie1a
b0e36fechanges I forgot to commit a few weeks ago
b412977manual merge with 6.7.beta0
de60849I have no idea what I'm doing
0c28f5freverting parts of previous commit with fire
9830d63Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of git://trac.sagemath.org/sage into lie2

comment:9 Changed 5 years ago by git

  • Commit changed from 9830d63d19d1c4ec432a83ab8c21685ec7aae802 to 3d450840695003e8e410e0d0e3b926d7a00abe23

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

6c5fe38minor changes
3d45084fixing Jacobi identity for 3d example (unless Iam wrong? please add tests)

comment:10 Changed 5 years ago by darij

Just had a quick look at some of the examples. Please check my last commit and revert if necessary (I cannot test right now since Sage is compiling again).

comment:11 Changed 5 years ago by git

  • Commit changed from 3d450840695003e8e410e0d0e3b926d7a00abe23 to 5e9a839618ae034d04c13340603a8b62d4bbf2f8

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

d952a7bMerge branch 'public/categories/lie_algebras-16819' into 6.7.b1
dfad36ctrac #16819 wrong inclusion into doc is corrected
eb3b058hopefully fix doc bug
498af17Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of git://trac.sagemath.org/sage into lie2
5e9a839Jacobi identity now tested

comment:12 Changed 5 years ago by git

  • Commit changed from 5e9a839618ae034d04c13340603a8b62d4bbf2f8 to 005984dfd1e329136db896a7dd29c1a2153dcff6

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

005984ddocument the broken multiplication by base ring

comment:13 Changed 5 years ago by git

  • Commit changed from 005984dfd1e329136db896a7dd29c1a2153dcff6 to 5bf7cf7b377985c6374e1a907f517cae62c063ea

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

5bf7cf7documenting LieAlgebraWithStructureCoefficients (mostly scaffolding for myself)

comment:14 Changed 5 years ago by git

  • Commit changed from 5bf7cf7b377985c6374e1a907f517cae62c063ea to 4e8469da080ac7d303b74a8c697c6687b28e4dc3

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

4e8469dfixing own bugs, and heeding pyflakes

comment:15 Changed 5 years ago by git

  • Commit changed from 4e8469da080ac7d303b74a8c697c6687b28e4dc3 to 878ab1d0ed5dc2b0e27f1257caccb3747009bfb1

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

878ab1dmake generators work correctly for rank0 Heisenberg algebra

comment:16 Changed 5 years ago by git

  • Commit changed from 878ab1d0ed5dc2b0e27f1257caccb3747009bfb1 to 6f6c529b753631bde338b7225968619e193db794

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

6f6c529p-Witt algebras, so that we have a characteristic-p example

comment:17 Changed 5 years ago by git

  • Commit changed from 6f6c529b753631bde338b7225968619e193db794 to 052721a72568247cafc5abff310ad3478f999961

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

052721aI don't know why (and in what sense) three_dimensional covers "all 3-dim Lie algebras", so I weakened that claim. (Maybe it does over fields, but I somehow doubt that it works in general.) Also, corrected a bordercase failure for empty gens.

comment:18 Changed 5 years ago by git

  • Commit changed from 052721a72568247cafc5abff310ad3478f999961 to 4dd721a971b8606497cc046cf19e2706666cdf37

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

4dd721atrac #16820 fixing doc inclusion once again (no .py)

comment:19 Changed 5 years ago by git

  • Commit changed from 4dd721a971b8606497cc046cf19e2706666cdf37 to ee3edc49855a1dd98bca0fad32f38897c5d9a3df

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

ee3edc4attempt at using from_vector in _element_constructor_; this causes weird errors I have yet to understand

comment:20 Changed 5 years ago by darij

See my last commit. Errors:

**********************************************************************
File "src/sage/algebras/lie_algebras/lie_algebra_element.py", line 179, in sage.algebras.lie_algebras.lie_algebra_element.LieAlgebraElementWrapper.__ne__
Failed example:
    L.zero() == 0
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/algebras/lie_algebras/lie_algebra_element.py", line 181, in sage.algebras.lie_algebras.lie_algebra_element.LieAlgebraElementWrapper.__ne__
Failed example:
    L.zero() != 0
Expected:
    False
Got:
    True
**********************************************************************

Also, are you sure you meant to keep the dot in sage/algebras/lie_algebras/examples.?

comment:21 Changed 5 years ago by git

  • Commit changed from ee3edc49855a1dd98bca0fad32f38897c5d9a3df to eb81ce8a62555f2157475b4190190feaaaa86e6a

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

eb81ce8document brokenness

comment:22 Changed 5 years ago by git

  • Commit changed from eb81ce8a62555f2157475b4190190feaaaa86e6a to 87a227f1248a94beabd834007d458a505e753e46

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

e37e0c7trac #16820 oops, forgot one dot
87a227fMerge branch 'public/lie_algebras/fd_structure_coeff-16820' with dot removal

comment:23 follow-up: Changed 5 years ago by tscrim

It at least covers all 3-dim Lie algebras over char 0 fields (up to isomorphism); I will check when I get home what generality they meant (and put a reference if you think it's necessary).

Also for this change:

+        if hasattr(self, "module") and x in self.module():
+            return self.from_vector(x)

I think you should do

try:
    if x in self.module():
        return self.from_vector(x)
except AttributeError:
    pass

For the FromAssociative.lift...well there's some ambiguity there as to whether we want to lift to the UEA or just some enveloping algebra. I opted to lift just to the defining algebra (which I forgot to change in the docs) as this seemed the most natural. We will also have to deal with subalgebras and a lift there.

For the tested methods, it was originally returning an element from a matrix Lie algebra, but through refactoring this had changed. This can be remedied by changing the 3 to 1, i.e.: L = lie_algebras.three_dimensional_by_rank(QQ, 1).

I'm also not happy with the corner cases being allowed as all properties become vacuous.

Anyways, I'll make changes tomorrow as I will let Sage compile tonight when I get back home.

comment:24 Changed 5 years ago by git

  • Commit changed from 87a227f1248a94beabd834007d458a505e753e46 to 8c5361b202dd68b31be6eb414b437badceb3fa76

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

8c5361bmeanwhile, fix _acted_upon_ bug

comment:25 in reply to: ↑ 23 ; follow-up: Changed 5 years ago by darij

  • Milestone changed from sage-6.6 to sage-6.7

Replying to tscrim:

It at least covers all 3-dim Lie algebras over char 0 fields (up to isomorphism); I will check when I get home what generality they meant (and put a reference if you think it's necessary).

Ah, please add the reference. That's quite an interesting result.

Also for this change:

+        if hasattr(self, "module") and x in self.module():
+            return self.from_vector(x)

I think you should do

try:
    if x in self.module():
        return self.from_vector(x)
except AttributeError:
    pass

Does this do something different or is it just more pythonic? I am worried about this try-except game as I can't tell whether the AttributeError? comes from self.module() or from the self.from_vector(x).

For the FromAssociative.lift...well there's some ambiguity there as to whether we want to lift to the UEA or just some enveloping algebra. I opted to lift just to the defining algebra (which I forgot to change in the docs) as this seemed the most natural. We will also have to deal with subalgebras and a lift there.

I am in favor of renaming it then. The abstract lift lazy_attribute in sage/categories/lie_algebras.py explicitly speaks of the UEA in its doc. A lift that can go anywhere depending on the concrete algebra will be rather useless.

For the tested methods, it was originally returning an element from a matrix Lie algebra, but through refactoring this had changed. This can be remedied by changing the 3 to 1, i.e.: L = lie_algebras.three_dimensional_by_rank(QQ, 1).

Nope, they are still not wrappers.

I'm also not happy with the corner cases being allowed as all properties become vacuous.

The corner cases need to be allowed, and I've already caught at least 2 bugs by inserting tests for them.

Last edited 5 years ago by darij (previous) (diff)

comment:26 Changed 5 years ago by git

  • Commit changed from 8c5361b202dd68b31be6eb414b437badceb3fa76 to 021ed1e344f8a30bc08407ac9069cb950429ff6d

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

021ed1emore tests

comment:27 in reply to: ↑ 25 ; follow-up: Changed 5 years ago by tscrim

Replying to darij:

Replying to tscrim:

It at least covers all 3-dim Lie algebras over char 0 fields (up to isomorphism); I will check when I get home what generality they meant (and put a reference if you think it's necessary).

Ah, please add the reference. That's quite an interesting result.

IIRC, it's mostly just doing some linear algebra, going rank by rank, and using the constraint by the Jacobi identity. However I will add it.

Also for this change:

+        if hasattr(self, "module") and x in self.module():
+            return self.from_vector(x)

I think you should do

try:
    if x in self.module():
        return self.from_vector(x)
except AttributeError:
    pass

Does this do something different or is it just more pythonic? I am worried about this try-except game as I can't tell whether the AttributeError? comes from self.module() or from the self.from_vector(x).

It is more pythonic (and faster, but not noticably). You are the one who wanted the rule of 3s contract, and because of the category almost every Lie algebra which has a module attribute also has a from_vector. Also I'd say the failure is more graceful this way as if either one is not implemented, then it results in an unable to do the coercion/conversion type error.

For the FromAssociative.lift...well there's some ambiguity there as to whether we want to lift to the UEA or just some enveloping algebra. I opted to lift just to the defining algebra (which I forgot to change in the docs) as this seemed the most natural. We will also have to deal with subalgebras and a lift there.

I am in favor of renaming it then. The abstract lift lazy_attribute in sage/categories/lie_algebras.py explicitly speaks of the UEA in its doc. A lift that can go anywhere depending on the concrete algebra will be rather useless.

Renaming which one? Also it's far from useless to lift to some enveloping algebra as you would want to convert between say, SGA and the Lie algebra.

For the tested methods, it was originally returning an element from a matrix Lie algebra, but through refactoring this had changed. This can be remedied by changing the 3 to 1, i.e.: L = lie_algebras.three_dimensional_by_rank(QQ, 1).

Nope, they are still not wrappers.

Whoops, yes. Do sl(QQ, 2, representation='matrix') or one of the upper triangular matrix Lie algebras.

I'm also not happy with the corner cases being allowed as all properties become vacuous.

The corner cases need to be allowed, and I've already caught at least 2 bugs by inserting tests for them.

No, they don't have to be allowed. In fact, because they are the empty set, they aren't really well-defined to me.

comment:28 Changed 5 years ago by git

  • Commit changed from 021ed1e344f8a30bc08407ac9069cb950429ff6d to c1838221b369f47e407ffcd95fd53bd4f239e21d

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

c183822all doctests pass again :)

comment:29 in reply to: ↑ 27 Changed 5 years ago by darij

Replying to tscrim:

IIRC, it's mostly just doing some linear algebra, going rank by rank, and using the constraint by the Jacobi identity. However I will add it.

A lot of Lie theory is just doing some linear algebra... Linear algebra does a lot.

It is more pythonic (and faster, but not noticably). You are the one who wanted the rule of 3s contract, and because of the category almost every Lie algebra which has a module attribute also has a from_vector. Also I'd say the failure is more graceful this way as if either one is not implemented, then it results in an unable to do the coercion/conversion type error.

You're right -- fixed!

For the FromAssociative.lift...well there's some ambiguity there as to whether we want to lift to the UEA or just some enveloping algebra. I opted to lift just to the defining algebra (which I forgot to change in the docs) as this seemed the most natural. We will also have to deal with subalgebras and a lift there.

So you have 3 different meanings of lift now:

  • lift from g to U(g);
  • lift from g to *some* associative algebra containing g;
  • lift from g to a bigger Lie algebra.

I have no idea how I am to rely on a method which can do either of these three things depending on the implementation (and possibly on the MRO?). These warrant distinct names.

Renaming which one?

Either.

Whoops, yes. Do sl(QQ, 2, representation='matrix') or one of the upper triangular matrix Lie algebras.

Thanks. And you know what's funny? The try/except change actually fixed the == 0 tests...

but caused some new crap:

sage: L = lie_algebras.sl(QQ, 2, representation='matrix')
sage: L(2)
[2 0]
[0 2]

This is not in sl(2)!

Although, this is tolerable due to how this version of sl(2) is defined (as a generate of E, F and H).

No, they don't have to be allowed. In fact, because they are the empty set, they aren't really well-defined to me.

0-dimensional Lie algebras exist and sometimes you get them by quotienting or restricting. It isn't mathematically reasonable to exclude them.


New commits:

c183822all doctests pass again :)
Last edited 5 years ago by darij (previous) (diff)

comment:30 Changed 5 years ago by tscrim

IIRC, how I got around the subalgebra lifting was calling it lift_ambient. Perhaps what we should do is have it take an optional argument which calls the respective coercion and has some default. Maybe ask Nicolas?

You're right, but it's the one consisting of {0}. However this is not the empty set like the set of 0x0 matrices the corner cases construct.

I'm getting on my flight now. Talk to you when I'm back in California.

comment:31 Changed 5 years ago by darij

  • Keywords sd67 added

lift_ambient sounds good -- now I'd just wish the other two lifts would have different names.

What do you mean by "not the empty set like the set of 0x0 matrices"? There is a unique 0x0 matrix, and it is strictly upper-triangular.

Yes, talk to you later, and thanks for all the quick responses so far -- they're very helpful.

Meanwhile, here is an enigma for other people to solve:

sage: L = lie_algebras.three_dimensional_by_rank(QQ, 3)
sage: L in Modules(QQ)
True
sage: L.zero() == 0
True
sage: L.zero() == QQ(0)
False

comment:32 Changed 5 years ago by git

  • Commit changed from c1838221b369f47e407ffcd95fd53bd4f239e21d to c75e37bbdc5a9208f04bd4b09a1f049aacf0bb32

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

dce923eFixing cases when the action should also precompose with a coercion.
6b2e9bbAdded test which checks the action map.
9b14aa5Change in docstring.
75232f0Merge branch 'public/coercion/fix_actions-18221' of git://trac.sagemath.org/sage into lie2
c75e37bwith trac 18221 merged in, some stopgaps removed

comment:33 Changed 5 years ago by git

  • Commit changed from c75e37bbdc5a9208f04bd4b09a1f049aacf0bb32 to 3032fc09f15c90b61a3cf9b146e90b000c15fa4c

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

3032fc0product of Lie algebra elements with base ring elements now doesn't lift into the universal envelope

comment:34 Changed 5 years ago by darij

Here is a problem with using lift for any kind of lift:

sage: L = lie_algebras.sl(QQ, 2, representation='matrix')
sage: E, F, H = L.gens()
sage: E
[0 1]
[0 0]
sage: F
[0 0]
[1 0]
sage: H
[ 1  0]
[ 0 -1]
sage: E * H
[ 0 -1]
[ 0  0]

But E * H should not be -E if it's really to mean the element E * H of the universal envelope.

Also, it's black on white (green on black?) here:

        def universal_enveloping_algebra(self):
            """
            Return the universal enveloping algebra of ``self``.

            EXAMPLES::

                sage: L = LieAlgebras(QQ).FiniteDimensional().WithBasis().example()
                sage: L.universal_enveloping_algebra()
                Noncommutative Multivariate Polynomial Ring in b0, b1, b2
                 over Rational Field, nc-relations: {}

            ::

                sage: L = LieAlgebra(QQ, 3, 'x', abelian=True)
                sage: L.universal_enveloping_algebra()
                Multivariate Polynomial Ring in x0, x1, x2 over Rational Field

            .. SEEALSO::

                :meth:`lift`
            """
            return self.lift.codomain()

The codomain of the lift has to be the UEA.

Last edited 5 years ago by darij (previous) (diff)

comment:35 Changed 5 years ago by git

  • Commit changed from 3032fc09f15c90b61a3cf9b146e90b000c15fa4c to bf2eb372d2135972a1e646d1120772da33dc8013

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

bf2eb37same optimization as in trac 17098, plus getting rid of todos

comment:36 Changed 5 years ago by git

  • Commit changed from bf2eb372d2135972a1e646d1120772da33dc8013 to 09281457c8cf34a075be3b72ae88296a75c54d93

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

0928145trivial edit

comment:37 follow-up: Changed 5 years ago by tscrim

  • Dependencies #16819 deleted

Sorry for the delay in getting back to you; traveling plus clearing off my built up work.

comment:29 I think the issue is that there isn't enough checking of coefficients and maybe a default assumption somewhere that algebras are unital? Perhaps it's also lifting up in some fashion? I'll take a look at that this week.

comment:31 I would say the set of 0x0 matrices is the empty set, not the set containing a unique element, the empty matrix. As for the oddity, I think we need a better/more uniform system for equality for things that behave like 0. At the very least, this is an issue with CombinatorialFreeModuleElement that deserves a separate ticket:

sage: C = CombinatorialFreeModule(ZZ, ['a','b'])
sage: C.zero() == 0
True
sage: C.zero() == QQ(0)
False

comment:34 Yes, that is definitely bad. I'm thinking we should have lift always return an element of the UEA and state that explicitly. For the lifting to the defining associative algebra, perhaps call that lift_associative?

comment:38 in reply to: ↑ 37 ; follow-up: Changed 5 years ago by darij

Sorry for the delay in getting back to you; traveling plus clearing off my built up work.

Don't worry. I'm again swamped in work, so the sorriness is mutual.

Replying to tscrim:

comment:29 I think the issue is that there isn't enough checking of coefficients and maybe a default assumption somewhere that algebras are unital? Perhaps it's also lifting up in some fashion? I'll take a look at that this week.

Forget about this part -- I've since noticed that you define sl(2) as a Lie subalgebra of an associative algebra generated by a subset, and since you don't have methods which compute such a thing exactly, there is no surprise that it accepts input too liberally.

comment:31 I would say the set of 0x0 matrices is the empty set, not the set containing a unique element, the empty matrix.

There are very good reasons for considering it a one-element set. Matrices of size m \times n correspond to morphisms R^n \to R^m. How many morphisms are there from R^0 to R^0 (that is, from 0 to 0) ? One -- the zero morphism.

As for the oddity, I think we need a better/more uniform system for equality for things that behave like 0. At the very least, this is an issue with CombinatorialFreeModuleElement that deserves a separate ticket:

sage: C = CombinatorialFreeModule(ZZ, ['a','b'])
sage: C.zero() == 0
True
sage: C.zero() == QQ(0)
False

+1.

comment:34 Yes, that is definitely bad. I'm thinking we should have lift always return an element of the UEA and state that explicitly. For the lifting to the defining associative algebra, perhaps call that lift_associative?

That's a good idea.

The Lie subalgebra of an associative algebra generated by a subset will, so far, have no UEA, since we don't compute its full ground set. But now you made me wonder if we really need the Lie subalgebra of an associative algebra generated by a subset... If we don't compute its ground set, and only let the user compute "inside" it, then why don't we just use the whole associative algebra as a Lie algebra? The generators seem to be useless...

Last edited 5 years ago by darij (previous) (diff)

comment:39 in reply to: ↑ 38 Changed 5 years ago by tscrim

Replying to darij:

Replying to tscrim:

comment:31 I would say the set of 0x0 matrices is the empty set, not the set containing a unique element, the empty matrix.

There are very good reasons for considering it a one-element set. Matrices of size m \times n correspond to morphisms R^n \to R^m. How many morphisms are there from R^0 to R^0 (that is, from 0 to 0) ? One -- the zero morphism.

I would say that this is a degenerate case of the general equality, but fair enough. More fun with corner cases that no one will likely construct except for corner case testing... Will fix.

As for the oddity, I think we need a better/more uniform system for equality for things that behave like 0. At the very least, this is an issue with CombinatorialFreeModuleElement that deserves a separate ticket:

sage: C = CombinatorialFreeModule(ZZ, ['a','b'])
sage: C.zero() == 0
True
sage: C.zero() == QQ(0)
False

+1.

This is #18251.

comment:34 Yes, that is definitely bad. I'm thinking we should have lift always return an element of the UEA and state that explicitly. For the lifting to the defining associative algebra, perhaps call that lift_associative?

That's a good idea.

The Lie subalgebra of an associative algebra generated by a subset will, so far, have no UEA, since we don't compute its full ground set. But now you made me wonder if we really need the Lie subalgebra of an associative algebra generated by a subset... If we don't compute its ground set, and only let the user compute "inside" it, then why don't we just use the whole associative algebra as a Lie algebra? The generators seem to be useless...

Then you'd have gln = sln and there will be Lie subalgebras eventually implemented. More generally, the UEA of the (Lie) subalgebra will be a (assoc) subalgebra of the UEA of the ambient Lie algebra. However, we will need general support for subalgebras, which I don't think #11111 provides...

comment:40 Changed 5 years ago by git

  • Commit changed from 09281457c8cf34a075be3b72ae88296a75c54d93 to 301ddb58fcbe191708c915f11985c37b73667922

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

4148fc2Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of git://trac.sagemath.org/sage into lie
301ddb5updating some doctests to recent changes in permutation groups (I guess)

comment:41 Changed 5 years ago by git

  • Commit changed from 301ddb58fcbe191708c915f11985c37b73667922 to 690e2f50e7d4d0b7446738a3a60bd0faa8bba540

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

690e2f5Merge branch 'public/lie_algebras/fd_structure_coeff-16820' into 6.8.b0

comment:42 Changed 5 years ago by chapoton

  • Status changed from needs_review to needs_work

one failing doctest, see patchbot report

comment:43 Changed 5 years ago by git

  • Commit changed from 690e2f50e7d4d0b7446738a3a60bd0faa8bba540 to 7a768e82f1eb1ccacf04169f8aadcbb4505eaae8

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

a7f9800Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of git://trac.sagemath.org/sage into public/lie_algebras/fd_structure_coeff-16820
53aec59Fixed trivial doctest failure.
7a768e8Reworking the UEA for LieAlgebraFromAssociative.

comment:44 Changed 5 years ago by tscrim

  • Status changed from needs_work to needs_review

I've fixed the issues with lift and the universal enveloping algebra for LieAlgebraFromAssociative.

Right now I'm going to leave the option to specify the generators when constructing a Lie algebra from an associative algebra. Once #17416 (subalgebras) is done, it will directly create the corresponding Lie subalgebra.

comment:45 Changed 5 years ago by git

  • Commit changed from 7a768e82f1eb1ccacf04169f8aadcbb4505eaae8 to 2f6b1e40e53f0039e5786a9390bdad1c9d2a7217

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

95ccdfcMerge branch 'public/lie_algebras/fd_structure_coeff-16820' into 6.9.b4
2f6b1e4trac #16820 correct syntax for input

comment:46 Changed 5 years ago by tscrim

Whoops :p, thanks Frederic.

comment:47 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

two failing doctests

comment:48 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.7 to sage-6.10

comment:49 Changed 4 years ago by git

  • Commit changed from 2f6b1e40e53f0039e5786a9390bdad1c9d2a7217 to 7c3978711e1ab0950e720b0bd7ba6e18c185114c

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

7c39787Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of trac.sagemath.org:sage into public/lie_algebras/fd_structure_coeff-16820

comment:50 Changed 4 years ago by git

  • Commit changed from 7c3978711e1ab0950e720b0bd7ba6e18c185114c to 15e231797c8028a56d66eb19b067df90f2822822

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

15e2317Fixing doctests due to output order.

comment:51 Changed 4 years ago by git

  • Commit changed from 15e231797c8028a56d66eb19b067df90f2822822 to b38f9838d5ebfcd120587543d60333c46ad3865a

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

b38f983documentation improvements

comment:52 Changed 4 years ago by git

  • Commit changed from b38f9838d5ebfcd120587543d60333c46ad3865a to 090d308cd47fb5dfec0acc935972af18e2164ded

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

090d308oops

comment:53 Changed 4 years ago by tscrim

  • Dependencies set to #17035

comment:54 Changed 4 years ago by git

  • Commit changed from 090d308cd47fb5dfec0acc935972af18e2164ded to eedbb63c2964f52cb513666e67d73b56adcb1752

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

a84a4baFixes for LieAlgebraFromAssociative.
eedbb63review of examples.py

comment:55 Changed 4 years ago by git

  • Commit changed from eedbb63c2964f52cb513666e67d73b56adcb1752 to 60031718f3b44571e9f83593aa7673a5cbb997d3

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

22f8b7718411: proofreading
4a9fd81Trac 18411: merge Sage 6.10.beta0
525d038Trac 18411: Fix doctest in colored_permutations.py
41af762Trac 18411: pass keywords in __call__
6e27ddeTrac 18411: merge sage-6.10.beta1
b21396cMerge branch '18411' into HEAD
b32e479further changes (mostly doc)
dbb5789Further fixes
e2fb14aremove _test_keytype since submodules of free modules are currently failing it
6003171Merge branch 'public/categories/cleanup_CFM_modules_wBasis-18066' of trac.sagemath.org:sage into public/lie_algebras/fd_structure_coeff-16820

comment:56 Changed 4 years ago by tscrim

  • Dependencies changed from #17035 to #17035 #18411 #18066
  • Reviewers set to Darij Grinberg
  • Status changed from needs_work to needs_review

comment:57 Changed 4 years ago by git

  • Commit changed from 60031718f3b44571e9f83593aa7673a5cbb997d3 to 10ce8ff4c84248795e99f807aac2772c33e03cd6

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

10ce8ffFixing some things from recent positive review/merges.

comment:58 Changed 4 years ago by git

  • Commit changed from 10ce8ff4c84248795e99f807aac2772c33e03cd6 to cdb2e5b9476356aa9f91d372a51aa802921f0e62

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

cdb2e5breview heisenberg.py

comment:59 Changed 4 years ago by git

  • Commit changed from cdb2e5b9476356aa9f91d372a51aa802921f0e62 to eda8d898c413e6244f515d72f6cbdca4aa2a763e

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

eda8d89review virasoro.py

comment:60 Changed 4 years ago by git

  • Commit changed from eda8d898c413e6244f515d72f6cbdca4aa2a763e to be4a02edb5f542e8bb988243cdd80d4ff33b384f

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

0fc61b3Adding coercions for the Heisenberg algebra.
be4a02eMerge branch 'public/lie_algebras/fd_structure_coeff-16820' of trac.sagemath.org:sage into public/lie_algebras/fd_structure_coeff-16820

comment:61 Changed 4 years ago by git

  • Commit changed from be4a02edb5f542e8bb988243cdd80d4ff33b384f to b8be3f4298913132df18d3230186f93686561223

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

b8be3f4review heisenberg.py again; coercion still relies on unstaged changes to module morphisms

comment:62 Changed 4 years ago by tscrim

  • Dependencies changed from #17035 #18411 #18066 to #17035 #18411 #18066 #10672

comment:63 Changed 4 years ago by git

  • Commit changed from b8be3f4298913132df18d3230186f93686561223 to 6815f1c02c6a13cef96d542328d3248e06ca9fb9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6815f1creview heisenberg.py again; coercion still relies on unstaged changes to module morphisms

comment:64 Changed 4 years ago by git

  • Commit changed from 6815f1c02c6a13cef96d542328d3248e06ca9fb9 to 573e0748daeb844cb79792c08a5c851ea047e7f1

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

b8be3f4review heisenberg.py again; coercion still relies on unstaged changes to module morphisms
96004dfDarij...
d94f997Some last compatibility issues with modules with basis (hopefully).
c986ca3Merge branch 'public/categories/last_compatibility-10672' into public/lie_algebras/fd_structure_coeff-16820
573e074Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of trac.sagemath.org:sage into public/lie_algebras/fd_structure_coeff-16820

comment:65 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work
  • Summary changed from Implement ABCs for Lie algebras and finite dimensional given by structure cofficients to Implement ABCs for Lie algebras and finite dimensional given by structure coefficients

doc does not build

comment:66 Changed 4 years ago by git

  • Commit changed from 573e0748daeb844cb79792c08a5c851ea047e7f1 to 3f9a4a19a30d6749ab046160bc43988958c9490f

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

3f9a4a1Fixing the failing doctests and making the doc build.

comment:67 Changed 4 years ago by tscrim

  • Status changed from needs_work to needs_review

comment:68 Changed 4 years ago by git

  • Commit changed from 3f9a4a19a30d6749ab046160bc43988958c9490f to b9930e6c4f3f099baf341b58dc81d50a00df4986

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

b9930e6some changes to standardization of names

comment:69 Changed 4 years ago by darij

I feel I don't understand the correct meaning of the arguments in a LieAlgebra call very well. What is going wrong here: user error?

sage: L = LieAlgebra(QQ, 'x,y,z', {('x','y'):{'z':1}, ('y','z'):{'x':1}, ('z','x'):{'y':1}}, index_set=range(3))
sage: (x,y,z) = L.gens()
sage: L.is_abelian()
False
sage: L[x,y]
0
sage: L[y,z]
0
sage: L[z,x]
0
sage: L[x,y] == L.zero() == L[y,z] == L[x,y]
True

comment:70 Changed 4 years ago by tscrim

It is user error as the keys for the structure coefficients should be the index set. However, I could (should) change things so that it doesn't matter which you use.

comment:71 Changed 4 years ago by darij

Ah! You're right, this is user error. I got confused by the fact that the names come before the index_set.

comment:72 Changed 4 years ago by tscrim

However I'm fixing the input so that it mends the input to use the indexing set.

comment:73 Changed 4 years ago by tscrim

...in structure_coefficients.py.

comment:74 Changed 4 years ago by git

  • Commit changed from b9930e6c4f3f099baf341b58dc81d50a00df4986 to 60f743a2d8f8d684e4e636d6bdf329fae9967cb2

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

60f743aParsing the input so that it uses the indexing set.

comment:75 Changed 4 years ago by tscrim

With this last commit, we have:

sage: sage: L = LieAlgebra(QQ, 'x,y,z', {('x','y'):{'z':1}, ('y','z'):{'x':1}, ('z','x'):{'y':1}}, index_set=range(3))
sage: sage: (x,y,z) = L.gens()
sage: sage: L[x,y]
L[2]

comment:76 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.10 to sage-7.0

two failing doctests, see patchbot report

comment:77 Changed 4 years ago by git

  • Commit changed from 60f743a2d8f8d684e4e636d6bdf329fae9967cb2 to 50f4441019eeb080c7e700e2853f9b6aa564cf7c

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

a02380fMerge branch 'public/lie_algebras/fd_structure_coeff-16820' in 7.0.rc1
50f4441trac #16820 correct the spelling of Cartesian

comment:78 Changed 4 years ago by chapoton

It seems that one doctest in Virasoro is fragile, namely the order of the terms is not always the same. I have tried to correct this doctest according to a previous patchbot report, and now it is failing again.

comment:79 Changed 4 years ago by jdemeyer

  • Dependencies #17035 #18411 #18066 #10672 deleted
  • Milestone changed from sage-7.0 to sage-7.1
  • Status changed from needs_review to needs_work

Minor comment: when adding new files, please use the standard copyright template from http://doc.sagemath.org/html/en/developer/coding_basics.html#headings-of-sage-library-code-files.

comment:80 Changed 4 years ago by darij

  • Reviewers Darij Grinberg deleted

Removing my name from the reviewer field, since I'm afraid it creates a false impression that this ticket is being taken care of. I currently don't believe that I will find the time necessary for this until Summer :/

comment:81 Changed 4 years ago by tscrim

  • Reviewers set to Darij Grinberg

However you have done a fair amount of review of this ticket and deserve to be put on as a reviewer. I think your message saying you won't likely be able to get to this until summer is sufficient for anyone following this.

comment:82 Changed 4 years ago by git

  • Commit changed from 50f4441019eeb080c7e700e2853f9b6aa564cf7c to f1514bd21164a21c8ff90adf91bb036605fc1d85

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

f1514bdMerge branch 'public/lie_algebras/fd_structure_coeff-16820' into 7.3.b2

comment:83 Changed 4 years ago by git

  • Commit changed from f1514bd21164a21c8ff90adf91bb036605fc1d85 to a016182b1fcb8dcb4b29840be98436dda8531464

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

7d155ecFixing trivial doctest failures.
a016182Implementing a better way to compare indices.

comment:84 Changed 4 years ago by tscrim

  • Keywords days74 added
  • Milestone changed from sage-7.1 to sage-7.3

comment:85 Changed 4 years ago by chapoton

apparently, doc does not build.

comment:86 Changed 4 years ago by git

  • Commit changed from a016182b1fcb8dcb4b29840be98436dda8531464 to 6fcb3726142ab68451acc682d523b264bc1e319f

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

6fcb372Travis' fix for documentation

comment:87 Changed 4 years ago by aram.dermenjian

  • Status changed from needs_work to needs_review

I did some basic looking over things and everything seems to work. Still needs to have the mathematics looked at and then the code further tested.

comment:88 Changed 4 years ago by git

  • Commit changed from 6fcb3726142ab68451acc682d523b264bc1e319f to 00816c6f058fcc1603cd4d8fdb9db495841f8ea5

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

00816c6Merge branch 'public/lie_algebras/fd_structure_coeff-16820' into 7.3.b7

comment:89 Changed 4 years ago by chapoton

one doctest complains about deprecation of generator_cmp

comment:90 Changed 4 years ago by git

  • Commit changed from 00816c6f058fcc1603cd4d8fdb9db495841f8ea5 to d5e618b4517c8766181a276da80f4e34080f60b6

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

d5e618babc of Lie algebras, use key rather than cmp to sort ; and sorting_key

comment:91 Changed 4 years ago by git

  • Commit changed from d5e618b4517c8766181a276da80f4e34080f60b6 to e3133954dd66f164586e5528f5825bbedb6f9e3a

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

e313395Merge branch 'public/lie_algebras/fd_structure_coeff-16820' in 7.3.b9

comment:92 Changed 4 years ago by chapoton

Travis and Darij, do you think that this still needs a lot of work ? That would be great to have in sage !

comment:93 Changed 4 years ago by tscrim

I am in the process of doing a rewrite of the underlying implementation from using CFM to wrapping the usual free modules since all of the algorithms rely on linear algebra. However, this doesn't need much work beyond that; just more of a check of the doc and basic math. So I can postpone by rewrite to a followup ticket if you want to finish the review now.

comment:94 Changed 4 years ago by darij

I don't know how much work this needs :/ truth be told, I've only reviewed some of the more mathematical parts of it. I was planning to do the rest of it when I'm in Minneapolis (it is so much easier when the author is nearby), but if you (Frederic, or whoever else) beat me to it, I'd certainly be thankful :)

comment:95 Changed 4 years ago by git

  • Commit changed from e3133954dd66f164586e5528f5825bbedb6f9e3a to e6851853a6de6e65cefabcd27b7ec20bf5ecfb25

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

224da75Merge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
8c0f1abMerge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
16e36a9Refactor to use free modules for fin-dim Lie algebras w/ structure coeffs; a bit more work needed still.
e685185Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of trac.sagemath.org:sage into public/lie_algebras/fd_structure_coeff-16820

comment:96 Changed 4 years ago by tscrim

Here's my work so far. There's still a few things that need to be fixed, but by far, the refactoring seems to be working. I hope to finish the rest over the next week or so.

comment:97 Changed 4 years ago by git

  • Commit changed from e6851853a6de6e65cefabcd27b7ec20bf5ecfb25 to 9f33f5d2da42d3c8619a1690d655579884418f2c

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

cd730d0Fixing universal enveloping algebra code.
0238567Cythonizing the element class.
9f33f5dFixing TestSuite for infinite-dimensional abelian Lie algebras.

comment:98 Changed 4 years ago by git

  • Commit changed from 9f33f5d2da42d3c8619a1690d655579884418f2c to 394c3f0eee14dc17129930c459d914c6252e5961

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

394c3f0Not creating a new vector at every step in _bracket_.

comment:99 Changed 4 years ago by tscrim

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to needs_work

Okay, that should fix all of the outstanding problems with my rebase. The only thing left is to add doctests, fix the docbuild, change the failing doctests (which are caused by those tests being for a different class), and remove any cruft that might have built up.

I think I will also add a class for infinite-dimensional Lie algebras that uses LieAlgebraElement (which should be renamed) that behaves somewhat like a CombinatorialFreeModule, but inherits from LieAlgebra. This will serve as another common ABC for the Heisenberg and Virasoro Lie algebras, loop algebras, etc.

I also Cythonized the element class (for Lie algebras w/ structure coefficients) and tried to make it so those are as fast as can be.

comment:100 Changed 3 years ago by git

  • Commit changed from 394c3f0eee14dc17129930c459d914c6252e5961 to 2e6692c257dbcf9b48252adc78adb41feb86de41

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

91c38a3Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of trac.sagemath.org:sage into public/lie_algebras/fd_structure_coeff-16820
2e6692cDoing some fixes and bringing it up to full coverage.

comment:101 Changed 3 years ago by git

  • Commit changed from 2e6692c257dbcf9b48252adc78adb41feb86de41 to af54ea281be2afdd7ddcc47527900ae152c111d3

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

af54ea2Cleaning up some (currently) unused imports.

comment:102 Changed 3 years ago by tscrim

  • Keywords days79 added
  • Status changed from needs_work to needs_review

Okay, back and fully ready for review.

comment:103 Changed 3 years ago by git

  • Commit changed from af54ea281be2afdd7ddcc47527900ae152c111d3 to d333710a1c9322000b5ae338ca037d3beb647a3e

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

d333710Omission of abelian Lie algebras from doc index, and citation fix.

comment:104 Changed 3 years ago by git

  • Commit changed from d333710a1c9322000b5ae338ca037d3beb647a3e to eff47f6b34d6b9e987912cf8236a895b77596413

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

eff47f6standardize_names_index_set: Fix doctest and better documentation

comment:105 Changed 3 years ago by mathzeta2

In comment:103 and comment:104 I reviewed the src/sage/structure/indexed_generators.py and src/sage/algebras/lie_algebras/abelian.py files, and corrected the minor errors I have found there.

Reviewing the entire ticket might be too much for me, but it would be great to have Lie algebras in Sage.

Last edited 3 years ago by mathzeta2 (previous) (diff)

comment:106 Changed 3 years ago by chapoton

there is an xrange, and many .iteritems (not good for py3)

comment:107 Changed 3 years ago by git

  • Commit changed from eff47f6b34d6b9e987912cf8236a895b77596413 to 965c70b1ef515c23bf6ca3be3ff6bf8a9d03c94b

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

f2b7cf2Merge branch 'public/lie_algebras/fd_structure_coeff-16820' in 7.5.b6
965c70btrac 16820 no more xrange

comment:108 Changed 3 years ago by git

  • Commit changed from 965c70b1ef515c23bf6ca3be3ff6bf8a9d03c94b to 7c92fd10fd6a4557c98e908d5d8051d57c9ac630

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

e8c5256Merge branch 'public/lie_algebras/fd_structure_coeff-16820' in 7.5.rc1
7c92fd1trac 16820 nonzero -> bool

comment:109 Changed 3 years ago by git

  • Commit changed from 7c92fd10fd6a4557c98e908d5d8051d57c9ac630 to 7bda1a49c73c19c3b18eb9fb0b07ece5f6323f65

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

a9d636cMerge branch 'public/lie_algebras/fd_structure_coeff-16820' in 7.6.b5
7bda1a4trac 16820 get rid of some .iteritems in py files

comment:110 follow-up: Changed 3 years ago by egourgoulhon

  • Reviewers changed from Darij Grinberg to Darij Grinberg, Eric Gourgoulhon

This ticket looks in good shape to me, with clear maths (as far as I can tell) and very high quality code. I've just a few comments:

  • The documentation of classes MatrixLieAlgebraFromAssociative and LieAlgebraMatrixWrapper is reduced to "Initialize self.", which certainly could be improved, even if the class names are quite explicit.
  • There is a typo in the reference [deGraaf2000]: "Mathemtaical" --> "Mathematical"
  • Some infinite-dimensional Lie algebras lack of a dimension method:
    sage: R = FreeAlgebra(QQ, 3)
    sage: dim(R)
    +Infinity
    sage: L = LieAlgebra(associative=R)
    sage: dim(L)
    AttributeError: 'LieAlgebraFromAssociative_with_category' object has no attribute 'dimension'
    
    Btw, shouldn't dimension be added to ParentMethods in the Lie algebra category?
  • Regarding Heisenberg Lie algebras:
    sage: L = lie_algebras.Heisenberg(QQ, 2)
    sage: L.lie_algebra_generators()
    Finite family {'p2': p2, 'q1': q1, 'p1': p1, 'q2': q2, 'z': z}
    sage: L.gens()
    (p1, p2, q1, q2)
    
    Shouldn't z be part of the tuple of generators returned by gens?
  • The patchbot is complaining about "Python 3 incompatible code" in lie_algebra_element.pyx, namely the use of iteritems. Has this not been fixed by from six import iteritems because it is a Cython file?
  • Just for curiosity: why are you constructing all Lie algebras by invoking the base class LieAlgebra, relying on LieAlgebra.__classcall_private__ to dispatch to the actual subclass corresponding to the input? Couldn't you use a function LieAlgebra to do this, as we did for manifolds (cf. the function Manifold)? (possibly renaming the class LieAlgebra to e.g. LieAlgebraBase to avoid any confusion). A drawback of the current setting is that the inline documentation obtained from LieAlgebra? returns the __init__ docstring:
       INPUT:
    
       * "R" -- the base ring
    
       * "names" -- (optional) the names of the generators
    
       * "category" -- the category of the Lie algebra; the default is
         the category of Lie algebras over "R"
    
    which does not correspond to the actual arguments to be passed to LieAlgebra (because of the __classcall_private__ mechanism) and therefore may confuse the user. If you want to keep LieAlgebra.__classcall_private__, a solution could be to set the INPUT section in the docstring of the LieAlgebra class.

comment:111 Changed 3 years ago by egourgoulhon

Another minor remark: the ticket title looks strange to me: shouldn't a name ("Lie algebras", I guess) be inserted behind "finite dimensional"?

comment:112 Changed 3 years ago by git

  • Commit changed from 7bda1a49c73c19c3b18eb9fb0b07ece5f6323f65 to cf88fee19593c1dc07b7e11a64a1b7cfa1c0f148

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

3e2c547Merge branch 'public/lie_algebras/fd_structure_coeff-16820' of git://trac.sagemath.org/sage into public/lie_algebras/fd_structure_coeff-16820
cf88feeChanges for the reviewer and using standard copyright.

comment:113 in reply to: ↑ 110 ; follow-up: Changed 3 years ago by tscrim

  • Description modified (diff)
  • Milestone changed from sage-7.4 to sage-8.0
  • Summary changed from Implement ABCs for Lie algebras and finite dimensional given by structure coefficients to Implement ABCs and categories for Lie algebras and finite dimensional Lie algebras given by structure coefficients

Replying to egourgoulhon:

This ticket looks in good shape to me, with clear maths (as far as I can tell) and very high quality code.

Thank you for doing the review.

  • The documentation of classes MatrixLieAlgebraFromAssociative and LieAlgebraMatrixWrapper is reduced to "Initialize self.", which certainly could be improved, even if the class names are quite explicit.

Fixed by adding a docstring stub.

  • There is a typo in the reference [deGraaf2000]: "Mathemtaical" --> "Mathematical"

Fixed.

  • Some infinite-dimensional Lie algebras lack of a dimension method:
    sage: R = FreeAlgebra(QQ, 3)
    sage: dim(R)
    +Infinity
    sage: L = LieAlgebra(associative=R)
    sage: dim(L)
    AttributeError: 'LieAlgebraFromAssociative_with_category' object has no attribute 'dimension'
    
    Btw, shouldn't dimension be added to ParentMethods in the Lie algebra category?

Fixed. Really it should belong to the ModulesWithBasis category (see #22629), but I moved it up to LieAlgebrasWithBasis category for now.

  • Regarding Heisenberg Lie algebras:
    sage: L = lie_algebras.Heisenberg(QQ, 2)
    sage: L.lie_algebra_generators()
    Finite family {'p2': p2, 'q1': q1, 'p1': p1, 'q2': q2, 'z': z}
    sage: L.gens()
    (p1, p2, q1, q2)
    
    Shouldn't z be part of the tuple of generators returned by gens?

I went the other way and dropped z from lie_algebra_generators since I wanted a more minimal generating set.

  • The patchbot is complaining about "Python 3 incompatible code" in lie_algebra_element.pyx, namely the use of iteritems. Has this not been fixed by from six import iteritems because it is a Cython file?

Yes, that is correct. I did some other small improvements from things I've learned about Cython recently.

  • Just for curiosity: why are you constructing all Lie algebras by invoking the base class LieAlgebra, relying on LieAlgebra.__classcall_private__ to dispatch to the actual subclass corresponding to the input? Couldn't you use a function LieAlgebra to do this, as we did for manifolds (cf. the function Manifold)? (possibly renaming the class LieAlgebra to e.g. LieAlgebraBase to avoid any confusion). A drawback of the current setting is that the inline documentation obtained from LieAlgebra? returns the __init__ docstring:

[snip]

which does not correspond to the actual arguments to be passed to LieAlgebra (because of the __classcall_private__ mechanism) and therefore may confuse the user. If you want to keep LieAlgebra.__classcall_private__, a solution could be to set the INPUT section in the docstring of the LieAlgebra class.

Because this is both the main entry point and the common base class, so there is no reason to separate the two IMO. It also avoids documentation duplication. Additionally the main docstring is (or at least should be) the class-level docstring, which has lots of different possible inputs first as examples. I agree it is a minor drawback that the __init__ does get included, but it is way at the bottom. However, the possible inputs are quite varied, so writing an INPUT block complicated.

comment:114 in reply to: ↑ 113 Changed 3 years ago by egourgoulhon

Replying to tscrim:

Replying to egourgoulhon:

  • Regarding Heisenberg Lie algebras: Shouldn't z be part of the tuple of generators returned by gens?

I went the other way and dropped z from lie_algebra_generators since I wanted a more minimal generating set.

I agree. Accordingly, it may be worth to change the documentation of HeisenbergAlgebra.z() to something like "the basis element z".

Still with Heisenberg algebras, we have

sage: L = lie_algebras.Heisenberg(QQ, 2)
sage: L.center()
AttributeError: 'HeisenbergAlgebra_with_category' object has no attribute 'basis_matrix'

Another issue arises because of the m[1] in line 136 of heisenberg.py::

sage: H = lie_algebras.Heisenberg(QQ, 10)
sage: p10 = H.gen(9); p10
p10
sage: latex(p10)
p_{1}

A drawback of the current setting is that the inline documentation obtained from LieAlgebra? returns the __init__ docstring, which does not correspond to the actual arguments to be passed to LieAlgebra (because of the __classcall_private__ mechanism) and therefore may confuse the user. If you want to keep LieAlgebra.__classcall_private__, a solution could be to set the INPUT section in the docstring of the LieAlgebra class.

Because this is both the main entry point and the common base class, so there is no reason to separate the two IMO. It also avoids documentation duplication. Additionally the main docstring is (or at least should be) the class-level docstring, which has lots of different possible inputs first as examples. I agree it is a minor drawback that the __init__ does get included, but it is way at the bottom.

Thanks for these explanations.

However, the possible inputs are quite varied, so writing an INPUT block complicated.

But isn't it the point of a reference manual to provide an exhaustive list of all possible inputs? Maybe it is too much in this case; if you think that the provided examples perform a sufficient coverage, then it is fine for me.

comment:115 Changed 3 years ago by darij

Returning for a little bit of bikeshedding:

  • "cosnstructed" in heisenberg.py.
  • There is a bunch of TODOs in lie_algebra_element.pyx. Are they all still up-to-date?
  • In lie_algebra_element.pyx, is self == 0 or y == 0 really the right thing? I thought we had is_zero() for testing zero without triggering a rube goldberg machine of coercions.
  • In lie_algebra_element.pyx, in class StructureCoefficientsElement method cpdef _bracket_(self, right), why not switch the next two statements:
    +                prod_c1_c2 = c1 * c2
    +                if not c2:
    +                    pass
    

Actually, is pass the right thing here, rather than continue? I thought pass is just a nop; or is this a Cython oddity? (Same for the if not c1 a few lines earlier.)

Last edited 3 years ago by darij (previous) (diff)

comment:116 Changed 3 years ago by git

  • Commit changed from cf88fee19593c1dc07b7e11a64a1b7cfa1c0f148 to 42666d4c3685d9ec2d9851ae9618d3a9e809449f

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

42666d4Adding centralizer_basis method, improving to/from_vector for finite-dim Lie algebras, other fixes.

comment:117 follow-up: Changed 3 years ago by tscrim

Fixes all of comment:114 and comment:115 (the only bikeshedding could be in the is_zero, but that might be a better approach, IDK exactly). Currently center will not work until subalgebra is implemented in generality, you can at least use L.centralizer_basis(L). The remaining TODO comments are the relevant ones.

comment:118 in reply to: ↑ 117 Changed 3 years ago by egourgoulhon

Thanks for the changes. IMO the ticket is ready for a positive review. Darij, do you agree?

comment:119 Changed 3 years ago by egourgoulhon

  • Status changed from needs_review to needs_work

Building the pdf ref. manual with ./sage -docbuild reference/categories pdf ends with:

! Extra }, or forgotten $.
l.29077 and recall that \(\mathfrak{g}_k}
                                          \supseteq \mathfrak{g}_{k+1}\).

Apparently, a { is missing at the left of the first k in line 560 of src/sage/categories/finite_dimensional_lie_algebras_with_basis.py. (sorry I haven't tried this sooner; on its side, ./sage -docbuild reference/algebras pdf is successfull).

comment:120 Changed 3 years ago by git

  • Commit changed from 42666d4c3685d9ec2d9851ae9618d3a9e809449f to 59df7ce401046c9bc1df6763d8ab80da594068a3

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

59df7ceFixing pdf docbuild in finite-dim Lie algebras w/ basis category.

comment:121 follow-up: Changed 3 years ago by tscrim

  • Status changed from needs_work to needs_review

Fixed the pdf docbuild.

comment:122 in reply to: ↑ 121 Changed 3 years ago by egourgoulhon

Replying to tscrim:

Fixed the pdf docbuild.

Thanks. I'm back in the positive review mood then.

comment:123 Changed 3 years ago by darij

Thanks for the fixes, Travis!

+        def product_space(self, L):
+            r"""
+            Return the product space ``[self, L]``.
+
+            EXAMPLES::
+
+                sage: L = LieAlgebras(QQ).FiniteDimensional().WithBasis().example()
+                sage: a,b,c = L.lie_algebra_generators()
+                sage: X = L.subalgebra([a, b+c])
+                sage: L.product_space(X)
+                An example of a finite dimensional Lie algebra with basis:
+                 the 0-dimensional abelian Lie algebra over Rational Field
+                 with basis matrix:
+                []
+                sage: Y = L.subalgebra([a, 2*b-c])
+                sage: X.product_space(Y)
+                An example of a finite dimensional Lie algebra with basis:
+                 the 0-dimensional abelian Lie algebra over Rational
+                 Field with basis matrix:
+                []
+
+            ::
+
+                sage: L.<x,y> = LieAlgebra(QQ, {('x','y'):{'x':1}})
+                sage: Lp = L.product_space(L) # todo: not implemented - #17416
+                sage: Lp # todo: not implemented - #17416
+                Subalgebra generated of Lie algebra on 2 generators (x, y) over Rational Field with basis:
+                (x,)
+                sage: Lp.product_space(L) # todo: not implemented - #17416
+                Subalgebra generated of Lie algebra on 2 generators (x, y) over Rational Field with basis:
+                (x,)
+                sage: L.product_space(Lp) # todo: not implemented - #17416
+                Subalgebra generated of Lie algebra on 2 generators (x, y) over Rational Field with basis:
+                (x,)
+                sage: Lp.product_space(Lp) # todo: not implemented - #17416
+                Subalgebra generated of Lie algebra on 2 generators (x, y) over Rational Field with basis:
+                ()
+            """
+            # Make sure we lift everything to the ambient space
+            try:
+                A = self._ambient
+            except AttributeError:
+                try:
+                    A = L._ambient
+                except AttributeError:
+                    A = self
+
+            B = self.basis()
+            LB = L.basis()
+            K = B.keys()
+            LK = LB.keys()
+            # We echelonize the matrix here
+            # TODO: Do we want to?
+            b_mat = matrix(A.base_ring(), [A.bracket(B[a], LB[b]).to_vector()
+                                           for a in K for b in LK])
+            b_mat.echelonize()
+            r = b_mat.rank()
+            I = A._basis_ordering
+            gens = [A.from_vector(row) for row in b_mat.rows()[:r]]
+            return A.subalgebra(gens)

I don't see why this should be a subalgebra (it is just a vector subspace). Counterexamples can easily be obtained if self (e.g.) is a free Lie algbera and L is the span of a single generator. (And that's just for the case when self and L both are Lie subalgebras; but the method is probably useful in higher generality.)

Why define I if you don't use it? And why define K and LK if you could just iterate over B and LB?

As for echelonization... It will, of course, break over non-fields. But I'm not sure if this function is supposed to apply to non-fields.

Last edited 3 years ago by darij (previous) (diff)

comment:124 Changed 3 years ago by tscrim

So the current implementation is based upon returning the subalgebra generated by gens, but indeed, this is not the right thing to do. So I should add something to check if L (and self) is an ideal or not. If it is, then the result is again an ideal.

As for non-fields, it should work (at least over nice enough rings):

sage: M = matrix([[1,2,5,2,3],[4,1,2,3,2],[6,2,3,1,2]])
sage: M.echelonize()
sage: M
[ 1  0  5 16  7]
[ 0  1  0 -7 -2]
[ 0  0  9 27 12]
sage: M.base_ring()
Integer Ring

comment:125 Changed 3 years ago by darij

Ah yes, it does work over the integers, though not over an arbitrary ring. Speaks in favor of an optional parameter.

I think there are good use cases for considering [U, V] when it is not a Lie ideal (let alone U and V). So maybe the result should really be just a subspace.

comment:126 Changed 3 years ago by git

  • Commit changed from 59df7ce401046c9bc1df6763d8ab80da594068a3 to ed587af6769afaae0dcd0ec9edd990f38c347cc0

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

ed587afImplementing ideal checks and fixing product_space.

comment:127 Changed 3 years ago by tscrim

I'm not sure about an optional parameter as that starts making things tricky because I think anything you would do likely has a call to echelonize somewhere.

I agree completely that we should allow [U, V] to not just be a Lie ideal, and I have implemented that in the last commit.

comment:128 follow-up: Changed 3 years ago by darij

Okay. Maybe worth explaining that thing with the base ring in the doc, though.

Something else:

+        # Do not override this. Instead implement :meth:`_construct_UEA`;
+        #   then, :meth:`lift` and :meth:`universal_enveloping_algebra`
+        #   will automatically setup the coercion

I might have already been nagging you about contracts and "what should I do if I want to set up a UEA for my own handmade Lie algebra" documentation. The above comment appears to explain it, but I'm not sure if it's enough. Literally it says I should just define _construct_UEA and the rest will happen automatically. But don't I also need to define lift, as it's otherwise just an abstract method in the base class? And is there a requirement that the base keys for the Lie algebra are a subset of the base keys for the UEA, or can a properly defined lift method work around this?

comment:129 Changed 3 years ago by git

  • Commit changed from ed587af6769afaae0dcd0ec9edd990f38c347cc0 to c0f01df449f2e40c26d6e8002c42f08fec59e589

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

3baf33fMerge branch 'develop' into public/lie_algebras/fd_structure_coeff-16820
c0f01dfCache is_ideal and explaining doc what is clear from the code.

comment:130 in reply to: ↑ 128 Changed 3 years ago by tscrim

Replying to darij:

Okay. Maybe worth explaining that thing with the base ring in the doc, though.

It's better and easier to let it fail in the appropriate way and place. Errors do serve a useful purpose and going crazy in the doc doesn't help, especially when the base code is subject to change without notice.

Something else:

+        # Do not override this. Instead implement :meth:`_construct_UEA`;
+        #   then, :meth:`lift` and :meth:`universal_enveloping_algebra`
+        #   will automatically setup the coercion

I might have already been nagging you about contracts and "what should I do if I want to set up a UEA for my own handmade Lie algebra" documentation. The above comment appears to explain it, but I'm not sure if it's enough. Literally it says I should just define _construct_UEA and the rest will happen automatically. But don't I also need to define lift, as it's otherwise just an abstract method in the base class? And is there a requirement that the base keys for the Lie algebra are a subset of the base keys for the UEA, or can a properly defined lift method work around this?

Answers to all of the above is no. It is actually more generic currently than I mentioned when we talked yesterday. It is clear from the code that one would need to define both _construct_UEA and the lift method for the elements (again, errors should result in the proper place(s)). However, I added a little more detail about this.

comment:131 Changed 3 years ago by darij

That clears things up, thanks!

There's a "the the" in the doc you just added. Aaaand no more nitpicking from me on this particular ticket :)

Last edited 3 years ago by darij (previous) (diff)

comment:132 Changed 3 years ago by git

  • Commit changed from c0f01df449f2e40c26d6e8002c42f08fec59e589 to fc395c8a0c03c608e2987b13dc0b8568e772046d

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

fc395c8Fixing typo.

comment:133 follow-up: Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

I am going to interpret that as a positive review (since I fixed the "the the"). Thank you very much to both of you!!!

comment:134 Changed 3 years ago by git

  • Commit changed from fc395c8a0c03c608e2987b13dc0b8568e772046d to 64ab6054b1316ce9faad4ec7b71ed0c0a3440d38
  • 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:

64ab605Trivial fix by making docstring a raw string.

comment:135 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Trivial change due to failing doctest from patchbot.

comment:136 in reply to: ↑ 133 Changed 3 years ago by egourgoulhon

Replying to tscrim:

I am going to interpret that as a positive review (since I fixed the "the the"). Thank you very much to both of you!!!

Thanks and congratulations for your work. This is a great enhancement arriving in Sage!

comment:137 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Print order doesn't seem to be fixed, on OSX:

sage -t --long src/sage/algebras/lie_algebras/virasoro.py
**********************************************************************
File "src/sage/algebras/lie_algebras/virasoro.py", line 418, in sage.algebras.lie_algebras.virasoro.VirasoroAlgebra.bracket_on_basis
Failed example:
    d.bracket_on_basis(2, -2)
Expected:
    -4*d[0] - 1/2*c
Got:
    -1/2*c - 4*d[0]
**********************************************************************
1 item had failures:
   1 of   4 in sage.algebras.lie_algebras.virasoro.VirasoroAlgebra.bracket_on_basis
    [59 tests, 1 failure, 1.51 s]

comment:138 Changed 3 years ago by darij

Could this be a general issue with the repr of combinatorial free modules whose basis is a DisjointUnionEnumeratedSets?

In other news, it just caught my eye that the overridden is_ideal on abelian Lie algebras tacitly assumes that the ambient Lie algebra of self is abelian, too (otherwise, self is not automatically a Lie ideal just by virtue of being abelian). I don't think this is clear to the user! I should have noticed this earlier :/ Travis, can you fix this or clarify my misunderstanding? Thank you!

comment:139 Changed 3 years ago by git

  • Commit changed from 64ab6054b1316ce9faad4ec7b71ed0c0a3440d38 to f8cb4764dce821a7076d3997a0345f6ce2443518

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

f8cb476Fix print order.

comment:140 Changed 3 years ago by tscrim

  • Status changed from needs_work to needs_review

It is an issue of comparing a string to an integer. I added a key function that fixes the problem.

The abelian Lie algebra in the category examples folder, where it is just a toy implementation showing basis features? It is not meant to be a full implementation, just to provide an example of a basic implementation.

comment:141 Changed 3 years ago by darij

Ah yes, that one. Still would be better if it would raise a NotImplementedError? instead.

comment:142 Changed 3 years ago by git

  • Commit changed from f8cb4764dce821a7076d3997a0345f6ce2443518 to 98c488db05e40d8fd099673e8f8274febc2a8657

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

98c488dMust be more restrictive when checking ideals.

comment:143 Changed 3 years ago by tscrim

For the record, I disagree as it should be a minimal implementation. However, I have changed it so it punts it up to the category code (which is technically unnecessary now because afterwards I implemented it at the category level). This made me realize we needed to be more restrictive with the is_ideal as it would run forever if given an infinite dimensional Lie algebra.

comment:144 Changed 3 years ago by darij

  • Status changed from needs_review to positive_review

Nice job with the basis keys -- back to positive review then!

comment:145 Changed 3 years ago by tscrim

Thank you!

comment:146 Changed 3 years ago by vbraun

  • Branch changed from public/lie_algebras/fd_structure_coeff-16820 to 98c488db05e40d8fd099673e8f8274febc2a8657
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.