Opened 9 months ago

Closed 3 months ago

#32505 closed enhancement (fixed)

Finitely presented graded modules over graded connected algebras

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-9.6
Component: algebra Keywords:
Cc: gh-sverre320, kvanwoerden, jhpalmieri, tscrim, gh-rrbruner, cnassau Merged in:
Authors: Bob Bruner, Michael Catanzaro, Sverre Lunøe-Nielsen, Koen van Woerden, John Palmieri, Travis Scrimshaw Reviewers: John Palmieri, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a1a9467 (Commits, GitHub, GitLab) Commit: a1a9467e95cd05a6439b4b2765e62c6aad6dc960
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (90)

comment:1 Changed 9 months ago by jhpalmieri

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__.

comment:2 Changed 9 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/free-graded-modules

comment:3 Changed 9 months ago by jhpalmieri

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

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

comment:4 Changed 9 months ago by jhpalmieri

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

comment:5 Changed 9 months ago by git

  • Commit changed from fa915966124a5e32b28a3a54538045d74858ed92 to fd8545b771c670f6eda373b3fa5e4ea88d17b45d

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

fd8545btrac 32505: clear out __init__.py

comment:6 Changed 9 months ago by jhpalmieri

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

comment:7 Changed 8 months ago by tscrim

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.

comment:8 Changed 8 months ago by git

  • Commit changed from fd8545b771c670f6eda373b3fa5e4ea88d17b45d to 5153c6fab78b1a9869b67c76fbc6c1fff81862f7

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:

comment:9 Changed 8 months ago by jhpalmieri

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.

comment:10 Changed 8 months ago by tscrim

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.

comment:11 follow-up: Changed 8 months ago by 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.

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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 8 months ago by tscrim

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.

comment:13 Changed 8 months ago by git

  • Commit changed from 5153c6fab78b1a9869b67c76fbc6c1fff81862f7 to b3816f980e1fe37b0cacc16273e96b0f66e01017

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

b3816f9trac 32505: minor cleanup

comment:14 in reply to: ↑ 12 Changed 8 months ago by jhpalmieri

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.

comment:15 Changed 8 months ago by git

  • Commit changed from b3816f980e1fe37b0cacc16273e96b0f66e01017 to ca45b2bfeb215d0d5e065daa3de1404c68467a85

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

comment:16 Changed 8 months ago by jhpalmieri

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

  • use __bool__ instead of __nonzero__
  • change the category for free modules to GradedModules(algebra).WithBasis(). I have not tested whether this allows me to get away with just defining degree_on_basis.
  • change the class for FPModule to inherit from IndexedGenerators, Module, and UniqueRepresentation.

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.

comment:17 Changed 8 months ago by git

  • Commit changed from ca45b2bfeb215d0d5e065daa3de1404c68467a85 to 46ef922ade27cd5223f64ae51238dc21813fa44a

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

46ef922trac 32505: change inheritance for FPModule

comment:18 Changed 8 months ago by jhpalmieri

Now including doctest coverage for everything.

comment:19 Changed 8 months ago by jhpalmieri

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.

comment:20 Changed 7 months ago by jhpalmieri

ping?

comment:21 Changed 7 months ago by tscrim

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.

comment:22 Changed 6 months ago by git

  • Commit changed from 46ef922ade27cd5223f64ae51238dc21813fa44a to b5c789541fab53e4163c636e646d59e6caa2bbed

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.

comment:23 Changed 6 months ago by jhpalmieri

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

  • I added test suites to the various __init__ methods, and now I'm getting a doctest failure that I have yet to track down. I would be happy for help:
    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
    
  • "Somewhere in the documentation needs to be stated that the module must be finite dimensional". We say "finitely presented" frequently already, but I've added a bit more. I also added a new category axiom, "FinitelyPresented".

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

comment:24 Changed 6 months ago by tscrim

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.

comment:25 Changed 6 months ago by git

  • Commit changed from b5c789541fab53e4163c636e646d59e6caa2bbed to e8e4927b68ba5f762f4d1ce13525bff38b3ca2f0

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

e8e4927trac 32505: add a few examples using exterior algebras.

comment:26 follow-up: Changed 6 months ago by jhpalmieri

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?

comment:27 follow-up: Changed 6 months ago by tscrim

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.

Last edited 6 months ago by tscrim (previous) (diff)

comment:28 follow-up: Changed 6 months ago by 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?

comment:29 follow-up: Changed 6 months ago by tscrim

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?

comment:30 Changed 6 months ago by tscrim

  • Branch changed from u/jhpalmieri/free-graded-modules to public/modules/free_graded_modules-32505
  • Commit changed from e8e4927b68ba5f762f4d1ce13525bff38b3ca2f0 to ded984f60dc7090448df317ec2b5104986ce1795
  • Reviewers set to John Palmieri, Travis Scrimshaw

New commits:

ded984fReviewer changes with misc tweaks and fixes.

comment:31 in reply to: ↑ 28 Changed 6 months ago by jhpalmieri

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

comment:32 Changed 6 months ago by git

  • Commit changed from ded984f60dc7090448df317ec2b5104986ce1795 to a12a8380fa33f8fb08c9c8b0cf9214c5a123861c

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

comment:33 Changed 6 months ago by git

  • Commit changed from a12a8380fa33f8fb08c9c8b0cf9214c5a123861c to 8529b6c0be23120ac6f087a5d9c04b805fdf008a

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

8529b6ctrac 32505: add _latex_term

comment:34 Changed 6 months ago by jhpalmieri

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.

comment:35 Changed 6 months ago by jhpalmieri

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

comment:36 Changed 6 months ago by git

  • Commit changed from 8529b6c0be23120ac6f087a5d9c04b805fdf008a to 8e54829b2c315f9fd9bd696b82eef78c33510f09

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

8e54829trac 32505: typos

comment:37 Changed 6 months ago by git

  • Commit changed from 8e54829b2c315f9fd9bd696b82eef78c33510f09 to db79c13f71ab358d0d403db0903b57253110b53c

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

c0ebfa1trac 32505: rename kernel_morphism to kernel_inclusion,
db79c13trac 32505: comments about submodules

comment:38 in reply to: ↑ 27 Changed 6 months ago by jhpalmieri

Replying to tscrim:

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.

Thanks for figuring that out!

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.

Fixed. I also suggested in a private email that another option would be to have a new class FpMorphismKernel which would inherit from FPModule but would also remember ambient module and the inclusion map. In the particular setting here, I think that the morphism is going to be used more than the kernel, so using f.kernel_inclusion() and occasionally f.kernel_inclusion().domain() looks better to me than f.kernel().inclusion() and occasionally f.kernel().

Last edited 6 months ago by jhpalmieri (previous) (diff)

comment:39 in reply to: ↑ 29 Changed 6 months ago by jhpalmieri

Replying to tscrim:

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.

This may require some other changes; in particular, every algebra (with basis? or every algebra?) should have a free_module method, but the free_module method for Clifford/exterior algebras just gives a rank 1 free module, the module underlying the algebra itself. Searching for free_module methods suggests that some behave this way while some allow the user to specify a basis. We should probably have something like underlying_free_module(self) and free_module(self, basis=...). The version in categories/rings.py, which might be viewed as the default method, is a little strange, since it returns not just a free module V over the ring R but also maps R -> V and V -> R. Such maps are not particularly canonical, so I don't know why they are part of the structure. Actually, it looks like this may only return a free module of rank 1. I don't know why it doesn't just call CombinatorialFreeModule(R, basis, ...) and allow for higher rank modules.

Maybe my point is, this looks messy enough that it should be deferred to another ticket. Alternatively we could ignore the mess and just add a free_graded_module method as you suggest, and try to clean up the ungraded case later. It will be a little awkward if free_module and graded_free_module behave so differently.

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.

The current code allows for the construction of a finitely presented module from a morphism between two free modules (mor.to_fp_module()). That could also be turned into a method for free modules, accepting a morphism and returning an f.p. module.

comment:40 Changed 6 months ago by git

  • Commit changed from db79c13f71ab358d0d403db0903b57253110b53c to 2e9b692503d169c9e4f5fa0ee0c84ca5b7b6a863

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

2e9b692trac 32505: fix some comparisons to None

comment:41 Changed 6 months ago by jhpalmieri

We could do this, for example:

  • src/sage/categories/graded_algebras_with_basis.py

    diff --git a/src/sage/categories/graded_algebras_with_basis.py b/src/sage/categories/graded_algebras_with_basis.py
    index e7ae68328e..8af922cd31 100644
    a b class GradedAlgebrasWithBasis(GradedModulesCategory): 
    8484        # Also, ``projection`` could be overridden by, well, a
    8585        # projection.
    8686
     87        def free_graded_module(self, generator_degrees, names=None):
     88            """
     89            Create a finitely generated free graded module over ``self``
     90
     91            This is only implemented when the ground ring is a field.
     92
     93            INPUTS:
     94
     95            - ``generator_degrees`` -- tuple of integers defining the
     96              number of generators of the module, and their degrees
     97
     98            - ``names`` -- optional, the names of the generators. If
     99              ``names`` is a comma-separated string like ``'a, b,
     100              c'``, then those will be the names. Otherwise, for
     101              example if ``names`` is ``abc``, then the names will be
     102              ``abc_{d,i}``.
     103
     104            By default, if all generators are in distinct degrees,
     105            then the ``names`` of the generators will have the form
     106            ``g_{d}`` where ``d`` is the degree of the generator. If
     107            the degrees are not distinct, then the generators will be
     108            called ``g_{d,i}`` where ``d`` is the degree and ``i`` is
     109            its index in the list of generators in that degree.
     110
     111            See :mod:`sage.modules.fp_graded.free_module` for more
     112            examples and details.
     113
     114            EXAMPLES::
     115
     116                sage: Q = QuadraticForm(QQ, 3, [1,2,3,4,5,6])
     117                sage: Cl = CliffordAlgebra(Q)
     118                sage: M = Cl.free_graded_module((0, 2, 3))
     119                sage: M.gens()
     120                (g_{0}, g_{2}, g_{3})
     121            """
     122            from sage.modules.fp_graded.free_module import FreeGradedModule
     123            return FreeGradedModule(self, generator_degrees, names)
     124
     125
    87126    class ElementMethods:
    88127        pass
    89128

comment:42 in reply to: ↑ 26 Changed 5 months ago by kvanwoerden

Replying to jhpalmieri:

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?

We like the printing and naming mechanism you propose, jhpalmieri!

comment:43 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:44 Changed 5 months ago by tscrim

Sorry for being slow about this.

I like your idea in comment:41.

Now for something closer to bikeshedding. I think we should print the elements as, e.g., g[0] following CFM. In addition, we could just pass everything off to IndexedGenerators (via CFM) and let that handle everything. This makes it more customizable. In order to do so, we would extend that to support the names option. How does this sound? I can take care of this if you want.

comment:45 Changed 5 months ago by git

  • Commit changed from 2e9b692503d169c9e4f5fa0ee0c84ca5b7b6a863 to c3bf4b88715dba4a2419e398cd30884b7f3f5fff

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

c3bf4b8trac 32505: add free_graded_module method to GradedAlgebrasWithBasis

comment:46 Changed 5 months ago by jhpalmieri

Here is a branch with comment:41 incorporated. If you can push a branch with names etc., that would be great!

comment:47 Changed 5 months ago by git

  • Commit changed from c3bf4b88715dba4a2419e398cd30884b7f3f5fff to 8435c780eea6a5d6749d80d914e20dc433301a1b

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

8435c78Reformatting output, simplifying basis construction, other misc changes.

comment:48 follow-ups: Changed 5 months ago by tscrim

I have pushed the changes with names (we will want the patchbot to get around to it to see if there are other (almost certainly trivial) doctests breaking across Sage from the internal changes).

I also made a number of other changes and improvements. One important one is I changed the internal representation of basis elements to match the "generic" (i.e., no names specified) printing indices. This is the most natural way to associate data to the basis element (IMO the only other natural way would just be to just use {0, ..., k} to index the basis elements). I also directly links the FP module to the free module's indices. This was also very useful for setting up the element printing.

I renamed the to_fp_module to as_fp_module for modules and fp_module for morphisms. If it was to_fp_module, I would expect some kind of map to be returned. I wanted the different names both to reflect the English and that modules and morphism are fundamentally different objects.

I have some design questions:

  • I think we should have the FPModule construction be based on the morphism since that is the data that is actually stored. The __classcall_private__ would then preparse the input data as needed.
  • We should remove the __contains__ of FPModule. I think this is suppressing an issue with comparing elements. Contrast this code with
    sage: Z4 = Zmod(4)
    sage: Z5 = Zmod(5)
    sage: Z5(3) in Z4
    False
    sage: Z5(3) == Z4(3)
    False
    
    which is using the generic __contains__. Right now I can't provide any advice on how to fix this as I haven't looked at this too closely yet.

I also have some questions regarding the formatting that are easy enough to fix/change, but I wanted to be set on them before updating the remaining doctests (there will be some trivial failures in the added files):

  • What do we want the default prefix to be? Previously it was g, but I was thinking G to reflect that it should be an element of the module G. Pure bikeshedding, and I don't care. I just chose something that I thought was reasonable, but I should poll for other opinions before setting everything.
  • How do we want to print when the generator degrees? Right now the unique are G[0] and the non-unique are G(0, 0) because that was the easiest to do. I can easily change the unique to G(0), and it is also relatively easy to change G(0, 0) to, e.g., G[0,0].
  • I propose changing the morphism output to match that of ring morphisms:
    sage: R.<x> = ZZ[]
    sage: S.<y> = ZZ[]
    sage: R.hom([y])
    Ring morphism:
      From: Univariate Polynomial Ring in x over Integer Ring
      To:   Univariate Polynomial Ring in y over Integer Ring
      Defn: x |--> y
    
    This is better when there are a lot of elements, it is consistent with other parts of Sage, no tuple-vs-list printing issues, and we likely can take advantage of other morphism display code.
Last edited 5 months ago by tscrim (previous) (diff)

comment:49 in reply to: ↑ 48 Changed 4 months ago by jhpalmieri

Replying to tscrim:

I have pushed the changes with names (we will want the patchbot to get around to it to see if there are other (almost certainly trivial) doctests breaking across Sage from the internal changes).

Great, thank you!

  • I think we should have the FPModule construction be based on the morphism since that is the data that is actually stored. The __classcall_private__ would then preparse the input data as needed.

Sounds okay to me, but I don't a strong opinion about it.

  • We should remove the __contains__ of FPModule. I think this is suppressing an issue with comparing elements.

I'll try it and see what happens, then get back to you.

  • What do we want the default prefix to be? Previously it was g, but I was thinking G to reflect that it should be an element of the module G. Pure bikeshedding, and I don't care. I just chose something that I thought was reasonable, but I should poll for other opinions before setting everything.

I think lowercase is better: g[0] looks more like an element to me than G[0].

  • How do we want to print when the generator degrees? Right now the unique are G[0] and the non-unique are G(0, 0) because that was the easiest to do. I can easily change the unique to G(0), and it is also relatively easy to change G(0, 0) to, e.g., G[0,0].

I don't care too much, but I have a slight preference for uniformity: square brackets in both cases?

  • I propose changing the morphism output to match that of ring morphisms:

Yes, sounds good.

comment:50 Changed 4 months ago by git

  • Commit changed from 8435c780eea6a5d6749d80d914e20dc433301a1b to d745357be5294371512684d2a23614d358c43a7b

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

a556c49Merge branch 'develop' into t/32505/public/modules/free_graded_modules-32505
d745357trac 32505: fix doctests, change print representation for morphisms

comment:51 Changed 4 months ago by jhpalmieri

I changed G[0] to g[0] but also changed G(0, 0) to g(0, 0) — it wasn't clear how to get brackets in the second case. I also changed the print representation for morphisms. I think I fixed all of the doctests, too.

comment:52 Changed 4 months ago by git

  • Commit changed from d745357be5294371512684d2a23614d358c43a7b to a73519c9f03e4b9d5f2461a55619ee8b6a09c7fb

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

a73519ctrac 32505: delete the method "__contains__" for FPModules

comment:53 in reply to: ↑ 48 ; follow-up: Changed 4 months ago by jhpalmieri

Replying to tscrim:

  • I think we should have the FPModule construction be based on the morphism since that is the data that is actually stored. The __classcall_private__ would then preparse the input data as needed.

I would be happy for you to take care of this. I ran into problems because I want to use generators and relations as arguments (as is currently done) but then have __classcall_private__ convert to a morphism and pass that to __init__, and I couldn't figure out a clean way to do that. I didn't try that hard, but it seems that __classcall_private__ and __init__ should have the same arguments, so they both need morphism, generators, and relations? I guess we could switch to using a factory to construct these modules instead, but I didn't try that.

I removed the __contains__ method, but I didn't know what you were referring to with "I think this is suppressing an issue with comparing elements."

With elements, it's easy for me to switch to g(0) and keep the current g(0, 0), but I can't figure out how to instead switch the second one to g[0, 0].

Otherwise, I'm happy with this.

comment:54 Changed 4 months ago by git

  • Commit changed from a73519c9f03e4b9d5f2461a55619ee8b6a09c7fb to ebcae245641d773e9a3961702345143fe757b16a

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

ebcae24trac 32505: fix a doctest

comment:55 Changed 4 months ago by tscrim

Sorry, I have been busy this past week. I should be able to do this today. It is simply a matter of adding a layer in the _repr_term method to convert the input to a list and then passing it up. Although I am thinking a better (long term) solution is to add another hook in IndexedGenerators for more broadly handling iterable input and use the general mechanics (so if someone wants to change the brackets to, say, {, it becomes a simple changing of the print options). This should also be quick to do.

comment:56 Changed 4 months ago by git

  • Commit changed from ebcae245641d773e9a3961702345143fe757b16a to 6b840715fc73c0fc528a6d48e2020dc86c682bef

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

4599fb5Merge branch 'develop' into public/modules/free_graded_modules-32505
7d6bdf9Merge branch 'public/modules/free_graded_modules-32505' of git://trac.sagemath.org/sage into public/modules/free_graded_modules-32505
6b84071Adding additional print option to IndexedGenerators for iterating through keys.

comment:57 Changed 4 months ago by git

  • Commit changed from 6b840715fc73c0fc528a6d48e2020dc86c682bef to d6f67cfe115757c2415c7c4755a10e6a096ae649

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

d6f67cfFix containment issues and add some more doctests.

comment:58 in reply to: ↑ 53 Changed 4 months ago by tscrim

Replying to jhpalmieri:

Replying to tscrim:

  • I think we should have the FPModule construction be based on the morphism since that is the data that is actually stored. The __classcall_private__ would then preparse the input data as needed.

I would be happy for you to take care of this. I ran into problems because I want to use generators and relations as arguments (as is currently done) but then have __classcall_private__ convert to a morphism and pass that to __init__, and I couldn't figure out a clean way to do that. I didn't try that hard, but it seems that __classcall_private__ and __init__ should have the same arguments, so they both need morphism, generators, and relations? I guess we could switch to using a factory to construct these modules instead, but I didn't try that.

A factory is overkill. The only thing needed is checking if the __classcall_private__ input is a morphism or not. Well, I am feeling lazy (forgive me!) and don't really care too much to simplify the input as the current version works albeit somewhat inefficiently since it converts from the morphism data and then reconstructs said morphism. Anyways, this isn't anything that needs to be done right now. If you want to do this, please go ahead.

I removed the __contains__ method, but I didn't know what you were referring to with "I think this is suppressing an issue with comparing elements."

I was getting errors for those doctest you removed. Using the default containment, you get this:

sage: from sage.modules.fp_graded.module import FPModule
sage: M = FPModule(SteenrodAlgebra(2), [0,1], [[Sq(4), Sq(3)]])
sage: N = FPModule(SteenrodAlgebra(2), [0], [[Sq(2)]])
sage: y = Sq(2) * N.generator(0)
sage: y in M
True

which I think is wrong as it is checking this:

sage: M(y) == y
True

So either it is the equality or the conversion that has a bug (possibly both). Taking another look, the _element_constructor_ was too permissive in how it was handling elements. I have fixed this, and it seems to resolve other issues I found with testing for this.

With elements, it's easy for me to switch to g(0) and keep the current g(0, 0), but I can't figure out how to instead switch the second one to g[0, 0].

I have done this following what I sketched in comment:55.

Otherwise, I'm happy with this.

I am too if my changes are good.

comment:59 Changed 4 months ago by git

  • Commit changed from d6f67cfe115757c2415c7c4755a10e6a096ae649 to f5ec2c1b87b21f4af85c95ca48d43a6c68301419

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

f5ec2c1trac 32505: do not use __class__(...), instead explicitly name the class

comment:60 Changed 4 months ago by jhpalmieri

I tried to modify __classcall_private__ to allow a morphism as input, but I ran into problems, which I have now mostly fixed. One minor problem is that the class is often initialized with the explicit keyword generator_degrees (as in FPModule(..., generator_degrees=...)), but it doesn't make sense to use that name for the argument anymore, so I removed those. Now we can easily change the name of the argument. Another problem is that some objects were constructed by calling self.__class__(...), and this usage seems to bypass __classcall_private__ and go straight to __init__. It seems like a bad idea anyway, so I changed those to just use FPModule(...).

I may try later to implement a morphism as input, but this will do for now.

Last edited 4 months ago by jhpalmieri (previous) (diff)

comment:61 Changed 4 months ago by git

  • Commit changed from f5ec2c1b87b21f4af85c95ca48d43a6c68301419 to ca9cdf08d263d417566d20f86387c69512408034

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

ca9cdf0trac 32505: fix one doctest

comment:62 Changed 4 months ago by jhpalmieri

This passes all tests for me now. Let's not worry about allowing a morphism as input to the FPModule constructor. I'd like to be able to move on to #30680. Travis, I'm happy with all of your work on this; thank you. Any objections to my most recent changes?

comment:63 Changed 4 months ago by git

  • Commit changed from ca9cdf08d263d417566d20f86387c69512408034 to 7dc91398f34cd0f21a3a4922e8330f596454ea4d

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

7dc9139trac 32505: remove an unneeded import

comment:64 Changed 4 months ago by git

  • Commit changed from 7dc91398f34cd0f21a3a4922e8330f596454ea4d to b7f9179ccb50b17cf867e5b11009594a5ba6d155

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

b7f9179Adding support for morphism input.

comment:65 follow-up: Changed 4 months ago by tscrim

I got out of my laziness and added support for taking a morphism input.

I also changed the FPModule.j parameter to the hidden _j.

Although looking again at the code, I have a few more questions (sorry!):

  • With allowing both input formats, do we need the from_free_module* methods? They seem more like clutter to me. I propose to remove them.
  • Related, I don't see why we want to create a free FPModule (instead of FreeGradedModule) in resolution(). Likely this is related to having different APIs, where methods likely just need to be added to the free version or perhaps some ABC needs to be created. We might want to even have the __classcall_private__ redirect to the free module where appropriate.
  • Should the FPModule belong to the category of ModulesWithBasis? I don't think this is guaranteed (over the base algebra). See also the comment below.
  • I don't agree with the comment before FPModule "vector spaces" is ill-defined (in particular, we don't require the base ring of the base algebra to be a field). This can be corrected fairly easily. However, I propose removing this as it doesn't contribute anything to (understanding) the code.

comment:66 in reply to: ↑ 65 Changed 4 months ago by jhpalmieri

Replying to tscrim:

I got out of my laziness and added support for taking a morphism input.

Great, thanks! I also added code to allow a free module as input, in which case it would return a finitely presented module with no relations. We can revisit this based on how we want to handle your question below about resolution.

I also changed the FPModule.j parameter to the hidden _j.

Good, makes sense.

  • With allowing both input formats, do we need the from_free_module* methods? They seem more like clutter to me. I propose to remove them.

Done.

  • Related, I don't see why we want to create a free FPModule (instead of FreeGradedModule) in resolution(). Likely this is related to having different APIs, where methods likely just need to be added to the free version or perhaps some ABC needs to be created. We might want to even have the __classcall_private__ redirect to the free module where appropriate.

Good question. Comments from the original authors? Why not just use free modules in resolution?

  • Should the FPModule belong to the category of ModulesWithBasis? I don't think this is guaranteed (over the base algebra). See also the comment below.

I deleted this.

  • I don't agree with the comment before FPModule "vector spaces" is ill-defined (in particular, we don't require the base ring of the base algebra to be a field). This can be corrected fairly easily. However, I propose removing this as it doesn't contribute anything to (understanding) the code.

I replaced it with another comment, pointing out that some of the methods assume that there is a vector space structure and a chosen basis.

comment:67 Changed 4 months ago by git

  • Commit changed from b7f9179ccb50b17cf867e5b11009594a5ba6d155 to 5c242e73c2685630cea27fda10d1b14632c179da

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

5c242e7trac 32505: remove from_free_module, from_free_module_morphism

comment:68 Changed 4 months ago by git

  • Commit changed from 5c242e73c2685630cea27fda10d1b14632c179da to 5e021535eb5917c9b65036db7c018b66729063e3

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

5e02153trac 32505: remove from_free_module, from_free_module_morphism

comment:69 Changed 4 months ago by git

  • Commit changed from 5e021535eb5917c9b65036db7c018b66729063e3 to f253e83ccaed4b920b62147878c65cb8a0c1e61b

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

f253e83trac 32505: allow the resolution to be made up of maps between

comment:70 Changed 4 months ago by git

  • Commit changed from f253e83ccaed4b920b62147878c65cb8a0c1e61b to 11b37258a9c3fa8989f0e7c42bd839b643014a2a

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

11b3725trac 32505: allow the resolution to be made up of maps between

comment:71 Changed 4 months ago by jhpalmieri

This now allows for the possibility of the terms in the resolution being instances of FreeGradedModuleMorphism (except for the initial map, which has codomain the original module). I've also added a (slow) method which will compute the top dimension of a finite-dimensional algebra, for use in determining when to stop computing the resolution. I've also added a computation of a free resolution of a trivial module over an exterior algebra.

comment:72 Changed 4 months ago by jhpalmieri

So maybe the choice for resolution was because the initial map has codomain M which is likely not free. With the new as_free=True option, the later maps are all converted to type FreeGradedModuleMorphism, and you could just omit the 0th term to get an honest (graded) free resolution out of it.

Maybe now we're done?

comment:73 Changed 4 months ago by git

  • Commit changed from 11b37258a9c3fa8989f0e7c42bd839b643014a2a to 6c1ab8dc9a5f2a932297bf5256dab88dbb5f27e1

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

6c1ab8dtrac 32505: more fixes

comment:74 Changed 4 months ago by jhpalmieri

Fixed a few more things: the free_graded_module method for algebras with basis wasn't handling the names argument correctly, and a few doctests were failing. I changed some of the documentation, too.

comment:75 Changed 4 months ago by tscrim

I was just running into that problem in comment:74 and about to fix it. Thanks. I will be pushing a bunch of changes shortly too.

comment:76 Changed 4 months ago by git

  • Commit changed from 6c1ab8dc9a5f2a932297bf5256dab88dbb5f27e1 to 50744642c4c07b3987729988501d7347b5286b9d

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

72e455eWe do not require the base algebra's base ring to be a field.
fcce24dMerge branch 'public/modules/free_graded_modules-32505' of git://trac.sagemath.org/sage into public/modules/free_graded_modules-32505
2e0518fUsing free modules where possible and improved compatibility.
6cf6d14Merge branch 'public/modules/free_graded_modules-32505' of git://trac.sagemath.org/sage into public/modules/free_graded_modules-32505
5074464Some last fixes and removing redundancy.

comment:77 Changed 4 months ago by tscrim

I took this a bit further and made everything in the resolution a FreeGradedModule. As I suspected, there were some compatibility issues to sort out, but I took care of all of the ones that came up in doctests (and a few others I saw along the way). There might be a few others (which will show up as missing methods), but we can fix them as we come across them.

Towards this, I made the free module morphism/homspace a subclass of the respective non-free version. This required some slight workaround, but the code is much cleaner IMO because there are no duplicated methods and the subclassing makes mathematical sense.

I tweaked the documentation to say "free module" basically everywhere we use "vector space" because we are not using the field properties AFAICS.

As of right now, I am happy with the code. Please check my changes. If they are good, then go ahead and set a positive review.

comment:78 follow-up: Changed 4 months ago by jhpalmieri

I saw that you replaced PlusInfinity() with infinity, so I made the same change a few other places. I also removed a "todo" about dealing with finite dimensionality rather than just finiteness, handling it the same way we did elsewhere in the code. If someone gets motivated (on another ticket), they can try to handle the case where self.base_ring().dimension() is not defined.

comment:79 Changed 4 months ago by git

  • Commit changed from 50744642c4c07b3987729988501d7347b5286b9d to ffe7179490704efe9157a6796cdbf2ae68f27c46

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

ffe7179trac 32505: replace "PlusInfinity()" with "infinity"

comment:80 Changed 4 months ago by jhpalmieri

  • Authors changed from Bob Bruner, Michael Catanzaro, Sverre Lunøe-Nielsen, Koen van Woerden, John Palmieri to Bob Bruner, Michael Catanzaro, Sverre Lunøe-Nielsen, Koen van Woerden, John Palmieri, Travis Scrimshaw
  • Status changed from needs_review to positive_review

comment:81 in reply to: ↑ 78 Changed 4 months ago by tscrim

Replying to jhpalmieri:

I saw that you replaced PlusInfinity() with infinity, so I made the same change a few other places.

Indeed, this is a micro-optimization since they are the same object, but infinity is already created (and nailed in memory).

I also removed a "todo" about dealing with finite dimensionality rather than just finiteness, handling it the same way we did elsewhere in the code.

Thank you. I had made that note for myself, but I forgot about it.

If someone gets motivated (on another ticket), they can try to handle the case where self.base_ring().dimension() is not defined.

I concur.

Thank you.

comment:82 Changed 4 months ago by gh-sverre320

Good question. Comments from the original authors? Why not just use free modules in resolution?

In the original version, the free module class was not intended for public use.

comment:83 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work
[sagemath_doc_html-none] [modules  ] The inventory files are in local/share/doc/sage/inventory/en/reference/modules.
[sagemath_doc_html-none] Error building the documentation.
[sagemath_doc_html-none] Traceback (most recent call last):
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/runpy.py", line 197, in _run_module_as_main
[sagemath_doc_html-none]     return _run_code(code, main_globals, None,
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/runpy.py", line 87, in _run_code
[sagemath_doc_html-none]     exec(code, run_globals)
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage_docbuild/__main__.py", line 2, in <module>
[sagemath_doc_html-none]     main()
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage_docbuild/__init__.py", line 1731, in main
[sagemath_doc_html-none]     builder()
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage_docbuild/__init__.py", line 776, in _wrapper
[sagemath_doc_html-none]     getattr(DocBuilder, build_type)(self, *args, **kwds)
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage_docbuild/__init__.py", line 136, in f
[sagemath_doc_html-none]     runsphinx()
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage_docbuild/sphinxbuild.py", line 323, in runsphinx
[sagemath_doc_html-none]     sys.stderr.raise_errors()
[sagemath_doc_html-none]   File "/home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage_docbuild/sphinxbuild.py", line 258, in raise_errors
[sagemath_doc_html-none]     raise OSError(self._error)
[sagemath_doc_html-none] OSError: /home/release/Sage/local/var/lib/sage/venv-python3.9.9/lib/python3.9/site-packages/sage/modules/fp_graded/free_homspace.py:docstring of sage.modules.fp_graded.free_homspace.FreeGradedModuleHomspace.identity:4: WARNING: Literal block expected; none found.
[sagemath_doc_html-none] 
[sagemath_doc_html-none]     Note: incremental documentation builds sometimes cause spurious
[sagemath_doc_html-none]     error messages. To be certain that these are real errors, run
[sagemath_doc_html-none]     "make doc-clean doc-uninstall" first and try again.

comment:84 follow-up: Changed 4 months ago by jhpalmieri

Travis, was your intention to delete the identity method from free_homspace? Perhaps also remove zero?

comment:85 in reply to: ↑ 84 Changed 4 months ago by tscrim

Replying to jhpalmieri:

Travis, was your intention to delete the identity method from free_homspace? Perhaps also remove zero?

Yes it was. They are now inherited as they had (effectively) the same code as the not-necessarily-free homspace.

comment:86 Changed 4 months ago by git

  • Commit changed from ffe7179490704efe9157a6796cdbf2ae68f27c46 to a1a9467e95cd05a6439b4b2765e62c6aad6dc960

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

a1a9467trac 32505: remove redundant "zero" and "identity" methods

comment:87 Changed 4 months ago by jhpalmieri

  • Status changed from needs_work to positive_review

comment:88 Changed 4 months ago by tscrim

Thank you.

comment:89 Changed 4 months ago by jhpalmieri

Followup in #33275.

comment:90 Changed 3 months ago by vbraun

  • Branch changed from public/modules/free_graded_modules-32505 to a1a9467e95cd05a6439b4b2765e62c6aad6dc960
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.