sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.31k stars 449 forks source link

Finitely presented graded modules over graded connected algebras #32505

Closed jhpalmieri closed 2 years ago

jhpalmieri commented 2 years ago

This is a precursor to #30680, laying out the framework for finitely presented modules over graded connected algebras. #30680 will focus on the case of the Steenrod algebra, with specific applications in mind.

CC: @sverre320 @sagetrac-kvanwoerden @jhpalmieri @tscrim @rrbruner @cnassau

Component: algebra

Author: Bob Bruner, Michael Catanzaro, Sverre Lunøe-Nielsen, Koen van Woerden, John Palmieri, Travis Scrimshaw

Branch/Commit: a1a9467

Reviewer: John Palmieri, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32505

jhpalmieri commented 2 years ago
comment:1

I will quote from the TODO list in the forthcoming __init__.py:

  1. Why do we need to define __bool__ and __eq__ in element.py? They should be taken care of automatically, once we define __nonzero__.
  2. inhheritance: free modules, elements, etc., should perhaps inherit from fp versions, or maybe both should inherit from generic classes, to consolidate some methods (like degree, _repr_, others?)
  3. Test with graded modules over other graded rings. (Should be okay, but add some doctests.) In __classcall__/__init__ for FP_Modules, allow as input a free module or a morphism of free modules? Or just leave it as is, with methods in FP_Modules, morphisms, and free modules for these constructions. (See the pre-existing FP_Modules.from_free_module etc., and also new methods morphism.to_fp_module() and free_module.to_fp_module().)

Question 1 is bugging me the most: I get doctest failures if I don't define __bool__ and __eq__, but according to structure/element.pyx, I shouldn't have to do this: the documentation there for __nonzero__ says:

        Note that this is automatically called when converting to
        boolean, as in the conditional of an if or while statement.

which really sounds like I shouldn't have to define __bool__.

jhpalmieri commented 2 years ago

Branch: u/jhpalmieri/free-graded-modules

jhpalmieri commented 2 years ago

Commit: fa91596

jhpalmieri commented 2 years ago
comment:3

This is not the final draft — I'd like to resolve at least some of the "TODO"s — but I'll mark it as "needs review".


New commits:

fa91596trac 32505: finitely presented graded modules
jhpalmieri commented 2 years ago
comment:4

Structurally, the starting place for a review perhaps should be the free_* files; the others are built on top of those.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

fd8545btrac 32505: clear out __init__.py
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from fa91596 to fd8545b

jhpalmieri commented 2 years ago
comment:6

In order to follow #32501, __init__.py should be empty, so I changed it, moving the TODO list to module.py.

tscrim commented 2 years ago
comment:7

Thank you for separating this ticket out. It will make it easier to focus on the general functionality here. I finally have gotten back around to this. Sorry it took so long (but I have now finally moved to Japan).

I think we should change the name FP_Element and similar to FPElement to match PEP8.

There are many places where TESTS: should be TESTS::.

Let's get rid of the is_FreeGradedModuleHomspace and just replace it with the isinstance call.

I don't like the name free_element(). Perhaps free_module_representative()?

The mathematical description does not quite seem to match the implementation. Your basis elements are not a basis over the F-algebra A but over F. This needs to be very carefully explained in the documentation.

I think more things should be using the category framework (unless this becomes a significant bottleneck in #30680) Mainly I am looking at degree() by using degree_on_basis, but there is a mismatch with what this is a module over. Actually, this is more like the cartesian_product with some additional functionality. I might need to think a bit more about how this will all fit together.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from fd8545b to 5153c6f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

20aba35trac 32505: finitely presented graded modules
1ab07abtrac 32505: clear out __init__.py
5153c6ftrac 32505:
jhpalmieri commented 2 years ago
comment:9

Here are some of the changes: everything you suggested except for the category framework and changing the documentation to reflect what we mean by basis. (I thought I would take care of the easy things first.)

If you have more thoughts about how to use the category framework, I would be happy to hear them.

tscrim commented 2 years ago
comment:10

To utilize the degree() from the category framework, we only need to implement a degree_on_basis() method in the parent. This would mean less repeated code, although it might be slightly slower in the computation. Of course, the current version works and is fine to do things this way.

We also don't need the __nonzero__ anymore because __bool__ is Python3.

Right now, I feel like this is violating some internal assumptions of CFM because of the basis mismatch. So it should not inherit from CFM, but some other class, perhaps CombinatorialFreeModule_CartesianProduct as we realize it as Ak but are considering it to be an F-module from the implementation point of view. If we really want to think of it as an A-module, then internally we need to extract the F-basis coefficients from the values of the element dict (and we might want to think a bit about how we name our methods). This should be fairly simple to do, but requires some minor refactoring.

Based upon the code and its intended use, you are converting things a lot to/from (dense) vectors in Fd, so the Cartesian product approach with an entirely new element class might be the best option, where elements is stored as a dict of (degree, vector) pairs. I guess it depends on how much time is spent doing element manipulations like this, but the caching suggests this is a time-critical operation.

jhpalmieri commented 2 years ago
comment:11

First, FreeGradedModule is indeed an honest free module, and I think it should be okay to use CombinatorialFreeModule for it. The "basis" in this case is explicitly the basis as a module over algebra; it is not a vector space basis. As a result, the degree_on_basis() setup won't work, because it assumes that you're working with graded modules over an ungraded ring, and so it doesn't take into account the possible degree of the coefficients. At least that's my reading of the homogeneous_degree method in categories/filtered_modules_with_basis.py.

I don't really see how using Cartesian products will help: Ak is just a free module, so we should be able to use the free module class for it. I don't think the code will use the projection and inclusion maps that are provided by the Cartesian product class.

FPModule is trickier: it is not free as a module over algebra, but of course it is free over the ground field. I don't know of a suitable class in Sage for it, but CombinatorialFreeModule kind of works. One problem is that the basis keys correspond to the given choice of generators, and since the module need not be free, it need not be a basis. Another problem is that there is an actual vector space basis in each degree and we want to compute it, but we can't just deduce it from the "basis" for this CombinatorialFreeModule.

It's a good idea to maybe cache dense_coefficient_list for these elements, and maybe while we're at it, give the method for this class of elements a different name.

tscrim commented 2 years ago
comment:12

Replying to @jhpalmieri:

First, FreeGradedModule is indeed an honest free module, and I think it should be okay to use CombinatorialFreeModule for it. The "basis" in this case is explicitly the basis as a module over algebra; it is not a vector space basis. As a result, the degree_on_basis() setup won't work, because it assumes that you're working with graded modules over an ungraded ring, and so it doesn't take into account the possible degree of the coefficients. At least that's my reading of the homogeneous_degree method in categories/filtered_modules_with_basis.py.

Ah, right, because we are considering it over a graded ring, which we do not have a mechanism to take that into account. That is a missing feature of the categories that probably needs to be addressed at some point. However, then the category of GradedModulesWithBasis(R) is wrong, and instead it should be in GradedModules(R).WithBasis(). These are not the same

sage: GradedModulesWithBasis(QQ) == GradedModules(QQ).WithBasis()
False

as the latter is just saying there is a distinguished basis in a graded module, but not that the basis respects the grading. This allows us to circumvent this issue of the grading of the base ring (at least for now).

I don't really see how using Cartesian products will help: Ak is just a free module, so we should be able to use the free module class for it. I don't think the code will use the projection and inclusion maps that are provided by the Cartesian product class.

From the above, I agree that CFM is fine. So we don't have to use this.

FPModule is trickier: it is not free as a module over algebra, but of course it is free over the ground field. I don't know of a suitable class in Sage for it, but CombinatorialFreeModule kind of works. One problem is that the basis keys correspond to the given choice of generators, and since the module need not be free, it need not be a basis. Another problem is that there is an actual vector space basis in each degree and we want to compute it, but we can't just deduce it from the "basis" for this CombinatorialFreeModule.

We can weaken this to be a subclass of IndexedGenerators, Module, and UniqueRepresentation, which are the base classes of CFM and has mos of the desired features. There might be some features we might want to abstract from CFM to some intermediate ABC to also use in this class to remove code duplication. This will probably be the best way forward for FPModule.

It's a good idea to maybe cache dense_coefficient_list for these elements, and maybe while we're at it, give the method for this class of elements a different name.

If we are going to cache it, then we might as well only store that and reimplement the module structure following my proposal at the end of in comment:10. IIRC, it is faster to go from the dense list to the indexed free module element than the other way around.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

b3816f9trac 32505: minor cleanup
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 5153c6f to b3816f9

jhpalmieri commented 2 years ago
comment:14

Replying to @tscrim:

Ah, right, because we are considering it over a graded ring, which we do not have a mechanism to take that into account. That is a missing feature of the categories that probably needs to be addressed at some point. However, then the category of GradedModulesWithBasis(R) is wrong, and instead it should be in GradedModules(R).WithBasis(). These are not the same

sage: GradedModulesWithBasis(QQ) == GradedModules(QQ).WithBasis()
False

as the latter is just saying there is a distinguished basis in a graded module, but not that the basis respects the grading. This allows us to circumvent this issue of the grading of the base ring (at least for now).

First, it is unfortunate that these are not the same, but that's not something to be fixed here. What differences does it make in this particular case to use GradedModules(algebra).WithBasis()?

Re FPModule:

We can weaken this to be a subclass of IndexedGenerators, Module, and UniqueRepresentation, which are the base classes of CFM and has mos of the desired features. There might be some features we might want to abstract from CFM to some intermediate ABC to also use in this class to remove code duplication. This will probably be the best way forward for FPModule.

I'll work on this.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b3816f9 to ca45b2b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

1612a41trac 32505: change category for free modules
a569bf0trac 32505: use `__bool__` instead of __nonzero__
ca45b2btrac 32505: change inheritance for FPModule, first pass
jhpalmieri commented 2 years ago
comment:16

Here are a bunch of changes in response to Travis' suggestions:

I still need to add some doctests for methods copied over from CombinatorialFreeModule and other places, but this works as is. (So feel free to look at the changes, but I will be adding more doctests, if nothing else.) I have not created any intermediate classes in an attempt to remove any code duplication.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

46ef922trac 32505: change inheritance for FPModule
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ca45b2b to 46ef922

jhpalmieri commented 2 years ago
comment:18

Now including doctest coverage for everything.

jhpalmieri commented 2 years ago
comment:19

I think this all works. We can use the suggestion at the end of comment:10 now or defer to another ticket. I think I would prefer to wait and see where the real bottlenecks are.

jhpalmieri commented 2 years ago
comment:20

ping?

tscrim commented 2 years ago
comment:21

Sorry, I have been a bit busy with my move (Australia -> Japan).

Before this goes to a positive review, there are a number of little code polishing things to take care of:

  1. Full doctest coverage (mainly __init__ with adding a TestSuite(foo).run()).
  2. Pyflakes
  3. There are two == None -> is None needed.
  4. I don't think we should have 0 be displayed as <>. I know this is consistent with displaying the other elements, but I think it should just be 0.
  5. The documentation of degree() is wrong with the output for 0.
  6. Class docstrings shouldn't have an OUTPUT:.
  7. Somewhere in the documentation needs to be stated that the module must be finite dimensional (over the algebra A). I think the category should also reflect this. It should be possible to remove this limitation, which shouldn't be too hard, but it is there currently.
  8. if len(self.generator_degrees()) == 0: -> if not self.generator_degrees():
  9. I am not sure we should use the name basis_elements as it is not giving basis elements over A, but I can't think of a better name right now. It should be specified that these are basis elements over the base ring R of A to make this clear. (Note that the direct sum is as additive groups (or R-modules), not as A-modules.
  10. We probably want to state that this code only works for algebras that have a graded distinguished basis.
  11. I would want some of the key methods to have doctests that work for other graded algebras. Anything that is of the form *Sym is good; same with NilCoxeter or Exterior (finite dimensional ones) or Shuffle.

I will probably also make a pass through this later for some code tweaks and improvements. However, before that, I want to talk a little bit more about the possible optimization in comment:10. Do we have any good ways to check the efficacy of such a change? Mainly, is there a computation that takes a few minutes that heavily uses this code? In terms of memory, it will likely be better because we are not storing the same information twice.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 46ef922 to b5c7895

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

320be0dtrac 32505: finitely presented graded modules
fab508atrac 32505: clear out __init__.py
c093016trac 32505:
b595530trac 32505: minor cleanup
9058e9atrac 32505: change category for free modules
c0093e2trac 32505: use `__bool__` instead of __nonzero__
dfe2387trac 32505: change inheritance for FPModule
b5c7895trac 32505: add axiom "FinitelyPresented" for modules and use it.
jhpalmieri commented 2 years ago
comment:23

Thanks, Travis. I've addressed most of these. I will add some doctests involving some other algebras like Exterior. Regarding your list:

File "src/sage/modules/fp_graded/module.py", line 156, in sage.modules.fp_graded.module.FPModule.__init__
Failed example:
    TestSuite(M).run()
Expected nothing
Got:
      Failure in _test_category:
      Traceback (most recent call last):
        File "/Users/palmieri/Desktop/Sage/git/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/misc/sage_unittest.py", line 297, in run
          test_method(tester=tester)
        File "sage/structure/element.pyx", line 722, in sage.structure.element.Element._test_category (build/cythonized/sage/structure/element.c:6825)
          tester.assertTrue(isinstance(self, self.parent().category().element_class))
        File "/usr/local/Cellar/python@3.9/3.9.8/Frameworks/Python.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 688, in assertTrue
          raise self.failureException(msg)
      AssertionError: False is not true
      ------------------------------------------------------------
      The following tests failed: _test_category
    Failure in _test_elements
    The following tests failed: _test_elements

When I actually run Sage and look at this, I think this is relevant information:

sage: from sage.modules.fp_graded.module import FPModule
sage: A3 = SteenrodAlgebra(profile=[4,3,2,1])
sage: M = FPModule(A3, [0,1], [[Sq(2), Sq(1)]])
sage: x = M.an_element()
sage: x.parent().category().element_class
<class 'sage.categories.category.JoinCategory.element_class'>
sage: isinstance(x, x.parent().category().element_class)  # _test_category asserts True
False

Regarding comment:10 and optimization, I think the followup ticket #30680 will be a good testing ground, since it will use this code heavily.

tscrim commented 2 years ago
comment:24

Sorry (yet again) for taking so long to get to this. So as you are surmising, either the category or the element does not seem to be created correctly. I will need to look at this in more detail. As a quick workaround if we want to merge this quickly, we can just skip that particular test. However, this does seem to be a more serious but subtle issue that we should address.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

e8e4927trac 32505: add a few examples using exterior algebras.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b5c7895 to e8e4927

jhpalmieri commented 2 years ago
comment:26

Here are a few examples with exterior algebras. Also in the previous version M.gen(0) would work when M was a free graded module but not a finitely presented module, so I've added gen as an alias for generator in the f.p. case, too. Same for gens.

I also disabled the failing TestSuite doctest. If we can figure out how to fix it, even better.

Finally, I am not happy with how elements are printed. Rather than <x, y>, I would rather see x*g_0 + y*g_1. Users should be allowed to name their generators, so you could also do

sage: M.<a,b,c> = FPModule(...)
sage: x*a + y*b
x*a + y*b

or

sage: M = FPModule(..., name='b', ...)  # not sure about this syntax
sage: x*M.gen(0)
x*b_0

gh-sverre320, kvanwoerden, gh-rrbruner: any comments?

tscrim commented 2 years ago
comment:27

I know what the problem is. The TestSuite is very useful:

-element_class = FPElement
+Element = FPElement

I will push a fix along with some other miscellaneous doc fixes one I make my way through all of the code.

One this that I do not like is submodule returning a morphism. This is very counterintuitive to me. I propose we rename this submodule_inclusion or submodule_embedding (other names welcome). Same for kernel. I know this is meant for homological algebra (+1 for that), but the method names should reflect their behavior IMO.

tscrim commented 2 years ago
comment:28

I am also +1 on changing the print (and latex) representation of elements to have user specified names. We can just use the framework of IndexedGenerators to handle the printing. We can also add an option to retain the current printing (as it matches what FreeModule does). John, what would (x + y)*a, where x,y are different basis elements of the algebra A, print as? x*a + y*a or (x + y)*a? Should we add such an option too?

tscrim commented 2 years ago
comment:29

I also have some thoughts about the construction hooks. These do not necessarily need to be address on this ticket, but it might be good to do it here. I think we should have some method added to the category of GradedAlgebrasWithBasis called free_graded_module and possibly add FreeGradedModule to the global namespace. This would serve as the main entry point.

Next, to construct a finitely presented module, I am thinking we should implement a quotient method or some other natural method to compute finitely presented modules as a 2-step procedure. Of course, we can easily add a finitely_presented_graded_module() or similar such method to allow a direct construction.

Thoughts?

tscrim commented 2 years ago

Changed branch from u/jhpalmieri/free-graded-modules to public/modules/free_graded_modules-32505

tscrim commented 2 years ago

Reviewer: John Palmieri, Travis Scrimshaw

tscrim commented 2 years ago

Changed commit from e8e4927 to ded984f

tscrim commented 2 years ago

New commits:

ded984fReviewer changes with misc tweaks and fixes.
jhpalmieri commented 2 years ago
comment:31

Replying to @tscrim:

I am also +1 on changing the print (and latex) representation of elements to have user specified names. We can just use the framework of IndexedGenerators to handle the printing. We can also add an option to retain the current printing (as it matches what FreeModule does). John, what would (x + y)*a, where x,y are different basis elements of the algebra A, print as? x*a + y*a or (x + y)*a? Should we add such an option too?

I've been trying out using the default for IndexedFreeModuleElement, and it uses parentheses: (x+y)*a + (y+z)*b (no spaces around + inside the parentheses). I would like to stick with the default. That means getting rid of _repr_ for the elements and adding _repr_term for the parents.

By the way, the general switch from <a,b> to a*g_{0} + b*g_{1} is my preference, but it is also consistent with how Sage prints elements in algebraic structures more generally. My current implementation allows for

M = FreeGradedModule(A, (0, 2))  -->  generators are g_{0}, g_{2}
M = FreeGradedModule(A, (0, 0, 2))  -->  generators are g_{0,0}, g_{0,1}, g_{2,0}: two indices since multiple generators in the same degree
M = FreeGradedModule(A, (0, 0, 2), names='c')  -->  use "c" instead of "g"
M = FreeGradedModule(A, (0, 0, 2), names='a, b, c')  -->  generators a, b, c: no subscripts
M.<a0,b0,c2> = FreeGradedModule(A, (0, 0, 2))  -->  generators a0, b0, c2: no subscripts, also define a0, b0, c2
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ded984f to a12a838

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

8dfde96trac 32505: delete `_repr_` for elements and instead use _repr_term
a12a838trac 32505: rename kernel() to kernel_morphism() and similar for
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

8529b6ctrac 32505: add _latex_term
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from a12a838 to 8529b6c

jhpalmieri commented 2 years ago
comment:34

Here is a branch that uses the new print representation and adds a LaTeX representation. It also changes submodule to submodule_inclusion, kernel -> kernel_morphism, cokernel -> cokernel_morphism.

jhpalmieri commented 2 years ago
comment:35

We can undo these changes if there are sound objections, of course.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 8529b6c to 8e54829

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

8e54829trac 32505: typos