Closed heluani closed 4 years ago
Branch pushed to git repo; I updated commit sha1. New commits:
a657919 | Minor doctesting and bot complaints. |
Thank you. This is much easier to review. This looks pretty good overall. Here are my comments:
In the examples.py
, those should each be in separate files and simply imported in.
You don't need the \
to break lines in docstrings.
Rather than importing each of the examples, you should do
lazy_import('sage.algebras.lie_conformal_algebras', 'examples', _as='lie_conformal_algebras')
That way you can access them all from the command line as lie_conformal_algebras.Foo
.
Then in the examples.py
module-level doc you can have a link to each file too.
Instead of putting the docstring in the __init__
, it is better to put it at the class level.
All methods need doctests. A good one for __init__
is
sage: F = Foo(bar)
sage: TestSuite(F).run()
For __classcall__
and __classcall_private__
, it is good to have a test with different input showing the result is the same object (or delegates to the correct class).
Error messages, per Python conventions, should not start with a capital letter nor end with a period/full-stop.
Don't be afraid to break the 80 char/line limit when it makes the code more readable.
A LieConformalAlgebra
should also inherit from Parent
.
You might want to add a _test_jacobimethod to the
LieConfirmalAlgebra` (or maybe to the category) that tests the Jacobi identity.
If you want to try and speed things up, changing lie_conformal_algebra_element.py
to a Cython file and the elements to cdef
classes can get you some more speed.
I would avoid NN
and instead use ZZ
and checking if the element is positive.
I don't understand why you have LieConformalAlgebraWithGenerators
and LieConformalAlgebraWithBasis
. I would just fold the latter into the former (in particular, a basis implies generators).
You can change @abstract_method(optional=False)
-> @abstract_method
and remove the raise
code.
Since the ConformalLieAlgebras
category inherits from Modules
, I don't think you need the SubcategoryMethods:
WithBasis
, Graded
, and Super
.
It might be better to split off some of those stub categories into separate files so they can have room to grow later on (and it has better separation-of-concerns that way too).
Replying to @tscrim: Thanks I'll try to take care of this tomorrow. I hope. There's only one point that worries me (below about inheriting from Parent)
You don't need the
\
to break lines in docstrings.
I thought I was using that only in the middle of a long sphinx directive like
:class:`this name<\
this.long.class>`
To adhere to PEP8... I'll look for those.
Rather than importing each of the examples, you should do
lazy_import('sage.algebras.lie_conformal_algebras', 'examples', _as='lie_conformal_algebras')
That way you can access them all from the command line as
lie_conformal_algebras.Foo
.
This is going to be heavy to change in the whole doctesting framework as I am currently calling those examples directly by their classnames. I thought there wasn't anything bad by lazily importing those classes, is there some performance reduction by importing them individualy?
Then in the
examples.py
module-level doc you can have a link to each file too.Instead of putting the docstring in the
__init__
, it is better to put it at the class level.All methods need doctests. A good one for
__init__
issage: F = Foo(bar) sage: TestSuite(F).run()
For
__classcall__
and__classcall_private__
, it is good to have a test with different input showing the result is the same object (or delegates to the correct class).Error messages, per Python conventions, should not start with a capital letter nor end with a period/full-stop.
Don't be afraid to break the 80 char/line limit when it makes the code more readable.
A
LieConformalAlgebra
should also inherit fromParent
.
I had this in previous code, but as soon as I moved to have LieConformalAlgebra_with_basis
inheriting from CombinatorialFreeModule
this started raising errors from UniqueRepresentation
. I'll see if I can reproduce this
You might want to add a _test_jacobi
method to the
LieConfirmalAlgebra` (or maybe to the category) that tests the Jacobi identity.
I thought perhaps adding the check right after initialization. Skew-symmetry is doable at the dictionary level in the __classcall_
as it is done now. But Jacobi is a pain. However, right after the call to CombinatorialFreeModule.__init__
the bracket can be converted to actual vectors and Jacobi should be testable. The problem is that as it is now, just setting the structure constants for E8 takes a while. Testing Jacobi would be minutes!
I'll add the method as you suggest and then perhaps a keyword to the classcall to test it?
If you want to try and speed things up, changing
lie_conformal_algebra_element.py
to a Cython file and the elements tocdef
classes can get you some more speed.I would avoid
NN
and instead useZZ
and checking if the element is positive.I don't understand why you have
LieConformalAlgebraWithGenerators
andLieConformalAlgebraWithBasis
. I would just fold the latter into the former (in particular, a basis implies generators).
That is not necessarily true, a basis is as a basis of R
-modules while the generators are as LCAs, which in particular is as R[T]
-modules. But mostly the point is that sometimes you do have generators but there aren't a particularly natural choice, while there is a natural basis. Take the N=2 vertex algebra or LCA where different sets of generators would be called different topological twists, while the basis is the same.
But I do agree that this will most probably not be ever used, so I don't mind collapsing those two classes together if you think is best.
You can change
@abstract_method(optional=False)
->@abstract_method
and remove theraise
code.Since the
ConformalLieAlgebras
category inherits fromModules
, I don't think you need theSubcategoryMethods:
WithBasis
,Graded
, andSuper
.
Ah thanks, perhaps I can get rid of WithBasis, but I think Graded and Super need to be implemented as they are here cause I need them to commute, otherwise they wont. They wont commute even with "FinitelyGenerated". This is in general correct, but I left a note in the category saying that we will never care for gradings that do not preserve the super structure, so Super graded LCA should be the same as graded super LCA. This forced me to have those calls.
It might be better to split off some of those stub categories into separate files so they can have room to grow later on (and it has better separation-of-concerns that way too).
Sure, perhaps the whole graded tree that has the most methods?
Replying to @heluani:
Replying to @tscrim: Thanks I'll try to take care of this tomorrow. I hope. There's only one point that worries me (below about inheriting from Parent)
You don't need the
\
to break lines in docstrings.I thought I was using that only in the middle of a long sphinx directive like
:class:`this name<\ this.long.class>`
To adhere to PEP8... I'll look for those.
For those, you can do
> :class:`this name
> <this.long.class>`
This is used in a number of places. It is good to be close to be PEP8, but even within PEP8, there is some flexibility.
Rather than importing each of the examples, you should do
lazy_import('sage.algebras.lie_conformal_algebras', 'examples', _as='lie_conformal_algebras')
That way you can access them all from the command line as
lie_conformal_algebras.Foo
.This is going to be heavy to change in the whole doctesting framework as I am currently calling those examples directly by their classnames. I thought there wasn't anything bad by lazily importing those classes, is there some performance reduction by importing them individualy?
I realize this, but when I make such changes, a big find-replace usually cuts out a lot of the work. While the lazy importing is good, it does add more objects to the global namespace. It might be reasonable to have multiple entry points for some of these classes. However, this reduces the number of potential name conflicts and makes tab completion a little better for the less (or different) specialized user.
A
LieConformalAlgebra
should also inherit fromParent
.I had this in previous code, but as soon as I moved to have
LieConformalAlgebra_with_basis
inheriting fromCombinatorialFreeModule
this started raising errors fromUniqueRepresentation
. I'll see if I can reproduce this
Hmm...that is strange. Was it an unable to resolve the MRO error? There might be an issue with having things in the wrong order for the MRO. I would have to actually see the error to know what to do, and I haven't tried to directly change your code yet.
You might want to add a
_test_jacobi
method to theLieConfirmalAlgebra
(or maybe to the category) that tests the Jacobi identity.I thought perhaps adding the check right after initialization. Skew-symmetry is doable at the dictionary level in the
__classcall_
as it is done now. But Jacobi is a pain. However, right after the call toCombinatorialFreeModule.__init__
the bracket can be converted to actual vectors and Jacobi should be testable. The problem is that as it is now, just setting the structure constants for E8 takes a while. Testing Jacobi would be minutes!I'll add the method as you suggest and then perhaps a keyword to the classcall to test it?
No, you don't call it from the __classcall__
. By having this method, it gets called automatically only when running TestSuite(foo).run()
, which should have such methods for checking consistency (and can be set to test a limited number of elements). See ZZ._test_associativity??
or _test_jacobi_identity
in the LieAlgebras
category (the former I think is a better implementation).
I don't understand why you have
LieConformalAlgebraWithGenerators
andLieConformalAlgebraWithBasis
. I would just fold the latter into the former (in particular, a basis implies generators).That is not necessarily true, a basis is as a basis of
R
-modules while the generators are as LCAs, which in particular is asR[T]
-modules. But mostly the point is that sometimes you do have generators but there aren't a particularly natural choice, while there is a natural basis. Take the N=2 vertex algebra or LCA where different sets of generators would be called different topological twists, while the basis is the same.But I do agree that this will most probably not be ever used, so I don't mind collapsing those two classes together if you think is best.
I completely agree that you can have a finitely generated algebra without it having a (distinguished) basis. However, right now you have the class LieConformalAlgebraWithGenerators
inheriting from the class LieConformalAlgebraWithBasis
, implying that any subclass of LieConformalAlgebraWithGenerators
must have a basis.
Since the
ConformalLieAlgebras
category inherits fromModules
, I don't think you need theSubcategoryMethods:
WithBasis
,Graded
, andSuper
.Ah thanks, perhaps I can get rid of WithBasis, but I think Graded and Super need to be implemented as they are here cause I need them to commute, otherwise they wont. They wont commute even with "FinitelyGenerated". This is in general correct, but I left a note in the category saying that we will never care for gradings that do not preserve the super structure, so Super graded LCA should be the same as graded super LCA. This forced me to have those calls.
You have to be a little careful between the Graded
and WithBasis
as they should not commute. You can have a graded algebra that has a basis that is not graded. For example, {1, 1 + x}
is a basis for the graded ring ZZ[x] / (x^2)
that is not graded. For commuting with FinitelyGenerated
, I am less sure if you can have a finitely generated algebra that has a grading that is not finitely generated as a graded algebra.
Also, I think you shouldn't enforce such things between super and graded artificially like that as a user later on down the road might want a grading that is not necessarily compatible with the super grading (I think the code can do this, although I have never actually tried).
It might be better to split off some of those stub categories into separate files so they can have room to grow later on (and it has better separation-of-concerns that way too).
Sure, perhaps the whole graded tree that has the most methods?
Probably. I leave that decision to you.
Replying to @tscrim:
Hmm...that is strange. Was it an unable to resolve the MRO error? There might be an issue with having things in the wrong order for the MRO. I would have to actually see the error to know what to do, and I haven't tried to directly change your code yet.
It was, and it is not throwing it now, so I suppose I had them in the wrong order.
No, you don't call it from the
__classcall__
. By having this method, it gets called automatically only when runningTestSuite(foo).run()
, which should have such methods for checking consistency (and can be set to test a limited number of elements). SeeZZ._test_associativity??
or_test_jacobi_identity
in theLieAlgebras
category (the former I think is a better implementation).
Ahh thanks I'll take a close look
That is not necessarily true, a basis is as a basis of
R
-modules while the generators are as LCAs, which in particular is asR[T]
-modules. But mostly the point is that sometimes you do have generators but there aren't a particularly natural choice, while there is a natural basis. Take the N=2 vertex algebra or LCA where different sets of generators would be called different topological twists, while the basis is the same.But I do agree that this will most probably not be ever used, so I don't mind collapsing those two classes together if you think is best.
I completely agree that you can have a finitely generated algebra without it having a (distinguished) basis. However, right now you have the class
LieConformalAlgebraWithGenerators
inheriting from the classLieConformalAlgebraWithBasis
, implying that any subclass ofLieConformalAlgebraWithGenerators
must have a basis.
But that's the other way around here: these different classes are at the implementation level and not the category level. And by (finitely) generated in the description we mean "central extension of freely (finitely) generated". So any generated algebra by definition acquires a basis, given by the derivatives of all the non-central generators plus the central generators.
The other way around is that it doesn't work. You can have an LCA with a chosen R-basis and not a chosen generating set.
You have to be a little careful between the
Graded
andWithBasis
as they should not commute. You can have a graded algebra that has a basis that is not graded. For example,{1, 1 + x}
is a basis for the graded ringZZ[x] / (x^2)
that is not graded. For commuting withFinitelyGenerated
, I am less sure if you can have a finitely generated algebra that has a grading that is not finitely generated as a graded algebra.Also, I think you shouldn't enforce such things between super and graded artificially like that as a user later on down the road might want a grading that is not necessarily compatible with the super grading (I think the code can do this, although I have never actually tried).
I understand that, but in this case it does not applies. De definition of graded for vertex algebras is that there exists an actual operator whose eigenvalues are the grading. This is almost always the zero mode of the conformal vector. This rules out gradings that are not compatible with any super structure.
For the "with basis", this was an implementation decision. At this point constructing submodules and quotients of infinite dimensional graded modules with finite dimensional graded pieces is not implemented in modules.with_basis.subquotients. I've implemented this for modules where the "basis indexes" provide a subset(energy=)
method that lists indices with a given grading.
Replying to @heluani:
Replying to @tscrim:
That is not necessarily true, a basis is as a basis of
R
-modules while the generators are as LCAs, which in particular is asR[T]
-modules. But mostly the point is that sometimes you do have generators but there aren't a particularly natural choice, while there is a natural basis. Take the N=2 vertex algebra or LCA where different sets of generators would be called different topological twists, while the basis is the same.But I do agree that this will most probably not be ever used, so I don't mind collapsing those two classes together if you think is best.
I completely agree that you can have a finitely generated algebra without it having a (distinguished) basis. However, right now you have the class
LieConformalAlgebraWithGenerators
inheriting from the classLieConformalAlgebraWithBasis
, implying that any subclass ofLieConformalAlgebraWithGenerators
must have a basis.But that's the other way around here: these different classes are at the implementation level and not the category level. And by (finitely) generated in the description we mean "central extension of freely (finitely) generated". So any generated algebra by definition acquires a basis, given by the derivatives of all the non-central generators plus the central generators.
The other way around is that it doesn't work. You can have an LCA with a chosen R-basis and not a chosen generating set.
If you have an R-basis, you have a natural distinguished generating set (as an R-algebra): the basis. Although it seems like you are meaning to say finite generating set or having some other condition(s), which I didn't see in the documentation. Is that correct?
You have to be a little careful between the
Graded
andWithBasis
as they should not commute. You can have a graded algebra that has a basis that is not graded. For example,{1, 1 + x}
is a basis for the graded ringZZ[x] / (x^2)
that is not graded. For commuting withFinitelyGenerated
, I am less sure if you can have a finitely generated algebra that has a grading that is not finitely generated as a graded algebra.Also, I think you shouldn't enforce such things between super and graded artificially like that as a user later on down the road might want a grading that is not necessarily compatible with the super grading (I think the code can do this, although I have never actually tried).
I understand that, but in this case it does not applies. De definition of graded for vertex algebras is that there exists an actual operator whose eigenvalues are the grading. This is almost always the zero mode of the conformal vector. This rules out gradings that are not compatible with any super structure.
Ah, I see. Okay, thanks. Although it already seems like graded and super commute:
sage: C1 = Modules(ZZ).Graded().Super()
sage: C2 = Modules(ZZ).Super().Graded()
sage: C1
Category of super modules over Integer Ring
sage: C2
Category of super modules over Integer Ring
sage: C1 is C2
True
sage: C1 = Algebras(ZZ).WithBasis().Graded().Super()
sage: C2 = Algebras(ZZ).WithBasis().Super().Graded()
sage: C1 is C2
True
For the "with basis", this was an implementation decision. At this point constructing submodules and quotients of infinite dimensional graded modules with finite dimensional graded pieces is not implemented in modules.with_basis.subquotients. I've implemented this for modules where the "basis indexes" provide a
subset(energy=)
method that lists indices with a given grading.
I still wouldn't enforce that. All you need to do in your own code is to make sure that you have the correct category. Perhaps I am misunderstanding something. However, submodules definitely needs some continued improvements.
There is #17367 which essentially introduces a category of infinite dimensional algebras with finite-dimensional graded components. However that has stalled due to naming conventions and a few other issues I think. We see this a lot in combinatorics (e.g., symmetric functions), and it would be useful to have in Sage.
Branch pushed to git repo; I updated commit sha1. New commits:
498e433 | Several Fixes to 30032 |
Replying to @tscrim: Hello, here's my attempt to cover many of the things you requested.
In the
examples.py
, those should each be in separate files and simply imported in.
Done, recovered the original files for each example and added a new ticket #30043 with the missing examples.
You don't need the
\
to break lines in docstrings.
I got rid of some by making the line longer than 80 columns, but had to keep others otherwise the links would not appear correctly in the html.
Rather than importing each of the examples, you should do
lazy_import('sage.algebras.lie_conformal_algebras', 'examples', _as='lie_conformal_algebras')
That way you can access them all from the command line as
lie_conformal_algebras.Foo
.
I admit that I hated this idea at first but it took only one sed line and it is way better to have them under the same namespace with TAB-completion, thanks I didn't know that would come for free.
Then in the
examples.py
module-level doc you can have a link to each file too.
Done, there's also a catalog in the html refs but that's almost empty in this ticket.
Instead of putting the docstring in the
__init__
, it is better to put it at the class level.
Done, that's what took most of my day. I remember reading an argument of yours regarding this in one of the tickets about Lie algebras. The Development guide should state something about this. It seems to me that it encourages the other way around.
All methods need doctests. A good one for
__init__
issage: F = Foo(bar) sage: TestSuite(F).run()
Thanks did this. In this ticket they all pass. In the vertex algebra one I had to cap the max_runs for some tests (going to 100 elements in an ideal of a vertex algebra can be very costly) and some others I commented them out cause they fail pickling, I'll have to deal with that.
For
__classcall__
and__classcall_private__
, it is good to have a test with different input showing the result is the same object (or delegates to the correct class).
I added some of these.
Error messages, per Python conventions, should not start with a capital letter nor end with a period/full-stop.
I think I corrected them all, hopefully I didn't miss many.
A
LieConformalAlgebra
should also inherit fromParent
.
Done, did so too for VertexAlgebra
and PoissonVertexAlgebra
in the other ticket. You were right I was puting Parent
before UniqueRepresentation
and that raised __mro__
problems.
You might want to add a _test_jacobi
method to the
LieConfirmalAlgebra` (or maybe to the category) that tests the Jacobi identity.
I didn't do this yet for lack of time, I should have this tomorrow.
If you want to try and speed things up, changing
lie_conformal_algebra_element.py
to a Cython file and the elements tocdef
classes can get you some more speed.
I was planning on moving all element classes to Cython in a separate ticket if/when they get merged, is that alright?
I would avoid
NN
and instead useZZ
and checking if the element is positive.
Done
I don't understand why you have
LieConformalAlgebraWithGenerators
andLieConformalAlgebraWithBasis
. I would just fold the latter into the former (in particular, a basis implies generators).
Regarding this issue: I think that was mostly a missuse of the name. I changed the name of the class and file to FreelyGeneratedLieConformalAlgebra
. Perhaps the "finitely" version should also be called "finitely_freely_generated_lie_conformal_algebra" but seemed too long so I kept it.
The algebras that those classes cover are central extensions of free R[T]
-modules. I think it's best to keep the free version separated from the "with basis" version cause eventually I might implement quotients and ideals (although for LCA they are boring).
You can change
@abstract_method(optional=False)
->@abstract_method
and remove theraise
code.
Thanks I did so in many places. Kept some with a descriptive message.
Since the
ConformalLieAlgebras
category inherits fromModules
, I don't think you need theSubcategoryMethods:
WithBasis
,Graded
, andSuper
.
Got rid of the calls to WithBasis and nothing broke, thanks.
It might be better to split off some of those stub categories into separate files so they can have room to grow later on (and it has better separation-of-concerns that way too).
Separated the Graded Tree.
I am applying everything you say here to the other ticket, and I've rebased everything to this ticket and #30043. As long as the topology is simple it should be fine.
Replying to @tscrim:
If you have an R-basis, you have a natural distinguished generating set (as an R-algebra): the basis. Although it seems like you are meaning to say finite generating set or having some other condition(s), which I didn't see in the documentation. Is that correct?
You are right, I only implemented algebras that are central extensions of free R[T] modules. I hope the comment in the previous reply clarifies this. I think it may be useful to keep the classes separated cause certainly quotients with basis may be implemented. Perhaps FinitelyGeneratedLieConformalAlgebra
should also be renamed to FinitelyAndFreelyGeneratedLCA
or something like that?
Ah, I see. Okay, thanks. Although it already seems like graded and super commute:
sage: C1 = Modules(ZZ).Graded().Super() sage: C2 = Modules(ZZ).Super().Graded() sage: C1 Category of super modules over Integer Ring sage: C2 Category of super modules over Integer Ring sage: C1 is C2 True sage: C1 = Algebras(ZZ).WithBasis().Graded().Super() sage: C2 = Algebras(ZZ).WithBasis().Super().Graded() sage: C1 is C2 True
I didn't get this for free on Vertex Algebras, I think I copied the trick from their classes.
For the "with basis", this was an implementation decision. At this point constructing submodules and quotients of infinite dimensional graded modules with finite dimensional graded pieces is not implemented in modules.with_basis.subquotients. I've implemented this for modules where the "basis indexes" provide a
subset(energy=)
method that lists indices with a given grading.I still wouldn't enforce that. All you need to do in your own code is to make sure that you have the correct category. Perhaps I am misunderstanding something. However, submodules definitely needs some continued improvements.
There is #17367 which essentially introduces a category of infinite dimensional algebras with finite-dimensional graded components. However that has stalled due to naming conventions and a few other issues I think. We see this a lot in combinatorics (e.g., symmetric functions), and it would be useful to have in Sage.
I'll try to make all calls consistent. My approach was to spend some time making sure that all categories where the same so that later I didn't need to worry about this issue. So I may have plenty of calls to different categories if I separate them right away.
Branch pushed to git repo; I updated commit sha1. New commits:
0a154de | Added missing docstrings |
I think I found all missing docstrings. I don't know what to do about the failing pyflakes cause those are the imports from examples.py
I can lazy import them, but examples itself is already lazily imported.
New commits:
0a154de | Added missing docstrings |
Thank you for renaming the class. Now I am happy with the class hierarchy. Perhaps FinitelyFreelyGeneratedLCA
for the name?
For the patchbot reports:
You can ignore the startup modules because you need the all.py
and __init__.py
to be imported.
For the pyflakes in the examples.py
, you can do what Frédéric did in the lie_algebras/examples.py
and just have an assert
statement to silence the warnings.
This will need to be addressed:
Missing doctests in algebras/lie_conformal_algebras/lie_conformal_algebra_with_structure_coefs.py: 3 / 4 = 75%
(You can check this locally by sage --coverage filename1.py filename2.py path/to/directory
.)
Branch pushed to git repo; I updated commit sha1. New commits:
4b848f5 | Split category files and other fixes |
This last commit splits the subcategory of LCAs with Basis and Finitely generated LCAs to their own files. It fixes the missing docstring (thanks a lot for that flag, I should now be able to commit only when there's 100% coverage). And should stop the pyflakes warnings.
It no longer enforces commutation of Super
and Graded
with WithBasis
but it does enforce that Super
commutes with Graded
. There are other minor changes.
Still missing is the test for the Jacobi condition.
Branch pushed to git repo; I updated commit sha1. New commits:
44bd42c | Fix errors introduced by rebase |
Branch pushed to git repo; I updated commit sha1. New commits:
e64eb93 | Added Jacobi test |
Branch pushed to git repo; I updated commit sha1. New commits:
cd50373 | Revert _test_jacobi to some_elements instead of all gens |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2ec6648 | Revert _test_jacobi to some_elements instead of all gens |
Reviewer: Travis Scrimshaw
Starting to make reviewer changes. I changed the categories so that a super LCA is a graded LCA. More still to do.
New commits:
c323dfc | Merge branch 'u/heluani/lie_conformal_algebras' of git://trac.sagemath.org/sage into u/heluani/lie_conformal_algebras |
e106792 | Attempt to simplify the categories and match other graded/super conventions. |
Changed branch from u/heluani/lie_conformal_algebras to public/lie_conformal_algebras/initial-30032
Changed branch from public/lie_conformal_algebras/initial-30032 to u/heluani/lie_conformal_algebras
This is my attempt to fix the categories files without too much cluttering.
There are a couple of methods that are repeated on the non-super and super classes, but I think this is unavoidable.
I thought of separating the super files into their own files, but since there will be always this overlap, I think it's safer to keep them together, so that it is clear that when you modify a ParentMethod the same method on the super class needs to be modified.
Finally I simplified a bit the element classes by using the methods provided from filtered_modules_with_basis and super_modules_with_basis.
EDIT: I also added your fixes in your branch, didn't want to edit on top of that one.
New commits:
3422bcd | Docstring fixing |
f668b8e | Rewiewer fixes |
cbd2319 | Make super Lie Conformal Algebras an independent category |
Thank you. I won't be able to look at them today, but maybe tomorrow.
Branch pushed to git repo; I updated commit sha1. New commits:
d00112e | Moved methods to implementation classes |
This new commit moves all repeated methods to implementation classes. The way I see this we have three options:
LieConformalAlgebras()
and LieConformalAlgebras().Super()
and
their subcategories (this is implemented in HEAD~
)HEAD
) or Category
. I would vote against 3. because this approach will require having an entire tree of categories for the combinations of WithBasis()
, FinitelyGenerated()
and Graded()
that have both ElementMethods
and ParentMethods
.
These classes are already in place for 2. so there isn't much cost into having the methods there. The pros are that there is no repetitions while the cons are that we are not enforcing implementation details at the category level. Since this is really a design decision I'll follow what you decide. I leave here my preference for 2. which seems cleaner. Perhaps one can add abstract methods to the categories for the purpose of documentation.
As a side note: since I think this motif should appear in Sage in almost any algebra for any operad, this would be solved if instead of having Category_over_base_ring)
we would have Category_over_monoidal_category
. This way we would pass LieConformalAlgebras(Modules(QQ))
or LieConformalAlgebras(Modules(QQ).Super())
and we would obtain the right categories. I see that there is a Category_over_base
which is a bit too general perhaps. If you think there might be interest on this I think this is easily doable and can open a different ticket. It does not look like much work. For example Category_over_tensor_category()
would work here. I could also adapt this to use simply Category_over_base()
.
Changed branch from u/heluani/lie_conformal_algebras to public/lie_conformal_algebras/initial-30032
Here is a new version that has the (H-)graded and super functors commuting. So now the next question is how much overlap will there be between the super and the non-super code? Also, how much overlap is there with the Lie algebra code?
Perhaps there is some misunderstanding with terminology. A stub class is a very short class that has very minimal implementation. A stub class should not have a lot of methods by definition. This kind of abstraction is very common: there is a common ABC that has nearly all of the actual implementation with subclasses that carry meaning through their existence with a few additional methods that are specific to those classes (possibly growing larger than a stub).
New commits:
d5a78f0 | Merge branch 'u/heluani/lie_conformal_algebras' of git://trac.sagemath.org/sage into public/lie_conformal_algebras/initial-30032 |
9f2309b | Making the super and (H-)graded commute. |
Thanks for your time on this. I am still recompiling but from a browse to this commit this looks fine by me, having their own class for LCAs().Graded() allows you to not duplicate that _repr_object_names
method.
As for the stub class. The problem is that most of the methods are shared between the super and non-super version. In this ticket alone, the difference between commits d00112e0082 and cbd2319dd12 (which is where I moved the methods from the categories to their implementation classes) has the methods some_elements
, ngens
, gen
, bracket
, _bracket_
, nproduct
, _nproduct_
, T
, index
. Now several of these methods are abstract methods that have no implementation at the category level, but still they have to be there doubled with their docstrings.
But most importantly, this ticket itself doesn't have much functionality yet. When #29610 is merged, these categories will have the methods for lifting and constructing universal enveloping vertex algebras. Thinking ahead, any method that does not explicitly involve the parity sign will have to be implemented in the stub category or be doubled.
Even more to the point: any decision that is made here, will have to be faced for the categories of vertex algebras and Poisson vertex algebras, these have many more methods than LCAs. My impression is that most methods will end up in the stub categories.
Therefore, we will need to have stub categories for each of the axioms implemented I think. A method index
only makes sense for LCAs and super LCAs with basis, so there will have to be a stub category AbstractLCAWithBasis
implementing this method that both LCAsWithBasis
and SuperLCAsWithBasis
inherit. This makes 8 new stub-classes for the combinations of Graded
, FinitelyGenerated
and WithBasis
. It gets worse if in the future we start implementing other subcategories like quotients, ideals, modules, etc. That's why I think that 2 in comment 25 is a bit cleaner.
Branch pushed to git repo; I updated commit sha1. New commits:
ab7d342 | Add common category for (super) Lie (conformal) algebras. |
Here is a version with the common ABC. There are still doctests to fix and polish to apply, but hopefully this gives you an idea of what it will look like. This means you can avoid the method duplication and it is in a common point.
I suspect you are thinking of having to do things that the category framework does naturally to prevent the "combinatorial explosion" of classes. The categories do not need to all have nice names; there is nothing wrong with having a join category (or at least there shouldn't be).
One thing that might be better is to have an actual class hierarchy of ABCs, but usually I want implementation specific things to go there and more general object methods to be in the category. Experimentation might still be needed.
I also removed all of the is_*
that simply checks the category. I think you might as well check the category in those cases. I haven't had time to look at the later ticket to see if this will cause slowdowns, but these methods seemed like a bit of clutter to me.
Hi Travis, thanks for the push again. Here are some comments to see if we converge slowly. I'll name them in the order that appear in the patch.
'FinitelyGeneratedAsBracketAlgebra' makes sense for Lie algebras, but less so for Lie Conformal algebras. My understanding is that this axiom should mean that the operation [,]
is R
-linear for a ring R
and any element can be constructed as linear combination with coefficients in R
of elements of the form [a_1,[a_2,[...,[a_{n-1},a_n]...]
for a_i
in a finite set of generators. This is true for a Lie algebra over R
but is not for a LCA over R
. Also replacing R
by R[T]
, linear combinations are finite, but the operation [,]
is not R[T]
-linear. I'll post something about this in #30233
I suggest keeping the changes to LieAlgebras
in a separate ticket.
I think for any category where the notion of FinitelyGenerated
is not ambiguous this should be implemented. So I think LieConformalAlgebras(R).FinitelyGenerated()
should call FinitelyGeneratedAsBracketAlgebras
in this version.
I agree with removing all the is_*
methods. It shouldn't be hard for me to fix the other tickets I think.
I suppose you want the common category for super LCA and LCAs to be something like BracketAlgebras
, let's say I call this BracketConformalAlgebras
. This is fine as I mentioned before, but my point continues to be that this approach would force me to construct 8 classes for the 8 combinations of BracketConformalAlgebras
with or without Graded
, FinitelyGenerated
and WithBasis
. This gets worse if eventually more axioms are added.
Replying to @heluani:
Hi Travis, thanks for the push again. Here are some comments to see if we converge slowly. I'll name them in the order that appear in the patch.
Hopefully the convergence is not too slow. Thank you for working with me as we try to figure out what the best practices are. This has been a great test of the system.
- 'FinitelyGeneratedAsBracketAlgebra' makes sense for Lie algebras, but less so for Lie Conformal algebras. My understanding is that this axiom should mean that the operation
[,]
isR
-linear for a ringR
and any element can be constructed as linear combination with coefficients inR
of elements of the form[a_1,[a_2,[...,[a_{n-1},a_n]...]
fora_i
in a finite set of generators. This is true for a Lie algebra overR
but is not for a LCA overR
. Also replacingR
byR[T]
, linear combinations are finite, but the operation[,]
is notR[T]
-linear. I'll post something about this in #30233
Thanks. One thing we could consider doing is abusing our terminology a bit, but that would be a hack. So I would much rather have a good system without any abuses.
- I suggest keeping the changes to
LieAlgebras
in a separate ticket.
I can extract that out as a dependency.
- I think for any category where the notion of
FinitelyGenerated
is not ambiguous this should be implemented. So I thinkLieConformalAlgebras(R).FinitelyGenerated()
should callFinitelyGeneratedAsBracketAlgebras
in this version.
I thought it was doing that. Admittedly, I am not quite sure what you are asking/suggesting here.
- I suppose you want the common category for super LCA and LCAs to be something like
BracketAlgebras
, let's say I call thisBracketConformalAlgebras
. This is fine as I mentioned before, but my point continues to be that this approach would force me to construct 8 classes for the 8 combinations ofBracketConformalAlgebras
with or withoutGraded
,FinitelyGenerated
andWithBasis
. This gets worse if eventually more axioms are added.
The category framework would normally take care of that for you with join categories. The only reason you would specifically implement them that I see is because they would have specific methods you want (which means you would have to implement them anyways) or you wanted the category to print really pretty and it wasn't doing that before (but I think most users won't really care about that, if they even see it).
The way I see it, having more complex class hierarchy does increase the maintenance burden, but not as much as having duplicate methods.
Replying to @tscrim:
Thanks. One thing we could consider doing is abusing our terminology a bit, but that would be a hack. So I would much rather have a good system without any abuses.
I think what I am suggesting in #30233 comment 10 or even the more radical comment 13 are mathematically correct and easy to implement in Sage. It would also solve this issue here and simplify the FinitelyGenerated
methods in the other tickets for vertex algebras.
I thought it was doing that. Admittedly, I am not quite sure what you are asking/suggesting here.
Ah I apologize perhaps it's doing that. I saw that this patch removes LieConformalAlgebras.SubcategoryMethods.FinitelyGenerated()
and I don't see how it gets replaced. But I guess now from your comment that it would be calling BracketConformalAlgebras.FinitelyGenerated()
it this one has one implemented in their subcategory methods.
The category framework would normally take care of that for you with join categories. The only reason you would specifically implement them that I see is because they would have specific methods you want (which means you would have to implement them anyways) or you wanted the category to print really pretty and it wasn't doing that before (but I think most users won't really care about that, if they even see it).
The way I see it, having more complex class hierarchy does increase the maintenance burden, but not as much as having duplicate methods.
With this last statement I agree, the worst situation is duplicate methods. But another solution is as it is now to have the methods in the implementation classes instead of the category classes. For example, GradedLieConformalAlgebraElement
implements nproduct
both for super algebras or usual LCAs.
However, I gather from all your messages that you would rather have the extra tree of stub categories. This is fine by me. I can code that quickly and implement the changes you have in this branch. However, I'll wait to see what we agree on the issue of 1. above, if my suggestion in #30233 is fine or if I should keep FinitelyGeneratedAsLieConformalAlgebra
or similar. As soon as that is agreed upon I'll push the tree of stub categories here
Sorry for taking so long to respond. So what I am thinking for now the best thing to do is to have everything at the implementation level for the FinitelyGenerated
. This is what I did for Lie algebras as I didn't need the larger scale, and I wanted everything to go through specific ABCs. It is relatively easily to lift stuff up to the category level, which adds to the flexibility. So this would also entail removing the FinitelyGenerated
categories and axioms.
TL;DR Let us sidestep the issue of #30233 for now and do finitely generated with ABCs.
Hopefully we can still squeeze this into 9.2.
Changed branch from public/lie_conformal_algebras/initial-30032 to u/heluani/lie_conformal_algebras
Sorry for the delay, you can imagine I've been mostly babysitting. This commit adds most of your changes and creates the stub categories LambdaBracketAlgebras
and their subcategories. Every method that can be implemented the same way in the super and non-super category goes in these categories. They are mostly empty now since this ticket does not have a lot of implemented methods, but they should start filling up once the remaining tickets are reviewed.
I removed all the is_xxx
methods. The docstring examples in the stub categories refer explicitly to the main categories, as in LieConformalAlgebras(QQ)
instead of using the non-exposed category LambdaBracketAlgebras(QQ)
to not encourage use of these categories and to explicit the usage of the methods at the same time.
I kept the axiom FinitelyGeneratedAsLambdaBracketAlgebra
to mean finitely generated as an R[T]
-module which is what is mostly used in the theory, but this should be easy to change later on if/when #30233 is discussed.
Last 10 new commits:
44bd42c | Fix errors introduced by rebase |
e64eb93 | Added Jacobi test |
2ec6648 | Revert _test_jacobi to some_elements instead of all gens |
3422bcd | Docstring fixing |
f668b8e | Rewiewer fixes |
cbd2319 | Make super Lie Conformal Algebras an independent category |
d00112e | Moved methods to implementation classes |
12faa10 | Revert "Moved methods to implementation classes" |
6c92608 | Implemented LambdaBracketAlgebras |
385fe0c | Moved methods to LambdaBracketAlgebras category |
This ticket is a bare-bones version of the Lie conformal algebra version of #29610 by request of the reviewer.
CC: @tscrim @heluani
Component: algebra
Author: Reimundo Heluani
Branch:
0f39045
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/30032