Closed catanzaromj closed 2 years ago
Changed branch from u/tscrim/fp_modules_steenrod-30680 to u/jhpalmieri/fp_modules_steenrod-30680
I'm not entirely happy with this, but it seems to work.
New commits:
1ad443a | trac 30680: fix inconsistent orderings in vector_presentation |
Branch pushed to git repo; I updated commit sha1. New commits:
bb0c961 | trac 30680: an example with an odd primary Steenrod algebra |
Here's a doctest. I checked this by hand through degree 56 (which is where the problem was before).
I'm not planning to make any more changes tonight. I am never sure of the right way to iterate through the terms in a combinatorial free module; I thought that map_item
might be right but I couldn't get it to work in this case. I think that we should only get monomials in the current code so leading_support
should give us the right index (the support should have just one element in it), but I'm not completely positive. Maybe this can be cleaned up.
Changed branch from u/jhpalmieri/fp_modules_steenrod-30680 to u/tscrim/fp_modules_steenrod-30680
Thank you for figuring out the issue.
Using the example in element_from_coordinates
sage: A = SteenrodAlgebra(2)
sage: from sage.modules.fp_graded.steenrod.module import SteenrodFPModule
sage: rels = [[Sq(1),0,0,0], [Sq(2),0,0,0], [Sq(4),0,0,0], [Sq(8),0,0,0],
....: [0,Sq(1),0,0], [0,Sq(2),0,0], [0,Sq(4),0,0], [Sq(31),Sq(14),0,0],
....: [0,Sq(20),0,0], [0,0,Sq(1),0], [0,0,Sq(2),0], [0,Sq(31),Sq(6),0],
....: [0,0,Sq(8),0], [0,0,0,Sq(1)], [0,0,Sq(31),Sq(2)], [0,0,0,Sq(4)], [0,0,0,Sq(8)] ]
sage: M = SteenrodFPModule(A, [0, 17, 42, 71], relations=rels)
sage: %prun res = M.resolution(2, top_dim=30, verbose=True)
the changes I just pushed make the time go from ~38s to ~17s on my laptop. From my understanding, a chokepoint is converting to/from dense vectors. This now once-and-forever fixes a number of basis orders.
Hopefully nothing fundamentally relies on the underlying algebra being a CFM
(just in the category of GradedModulesWithBasis
). If we encounter issues as we add further algebras, mainly CGAs, then we can make fixes here as necessary IMO.
We can re-enable the caching on the higher level method of basis_element()
, but I don't think it is called frequently enough to warrant the caching.
I made a new ticket for a bug in partitions #33323 that was exposed by this change.
Doctests now fully pass for me. So I am ready to set a positive review if you agree with my latest push.
Last 10 new commits:
cb9713c | Renaming min_presentation to minimal_presentation. |
b055dd8 | Adding minimal_presentation for free modules. |
08a92ba | Some various cleanup of doc, fixing trivial failures, adding additional doctests. |
76ebd8c | Remove Fields requirement and add simple test. |
2cc1196 | Adding more support for ZZ (and possibly PID) graded algebras. |
90cc883 | Merge branch 'u/jhpalmieri/fp_modules_steenrod-30680' of git://trac.sagemath.org/sage into u/tscrim/fp_modules_steenrod-30680 |
a1a1671 | Changing the conversion to/from vectors for speed. |
c340478 | Partitions of negative n is always 0. |
1e770aa | Merge branch 'public/combinat/negative_partition_card-33323' into u/tscrim/fp_modules_steenrod-30680 |
c21d498 | Some cleanup from previous commit with partial work. Fixing doctests. |
Changed dependencies from #32505, #33275 to #32505, #33275, #33323
Should the thematic tutorial be added to the thematic tutorial index, in addition to the "tutorial document tree"?
Changed branch from u/tscrim/fp_modules_steenrod-30680 to u/jhpalmieri/fp_modules_steenrod-30680
Some typos and other minor fixes. I want to do one or two more passes before declaring it done, but it's looking very good. Thanks for all of your work on it, Travis.
New commits:
271db3b | Merge branch 'develop' into t/30680/finitely_presented_modules_over_the_steenrod_algebra |
95e82b2 | Merge branch 'u/tscrim/fp_modules_steenrod-30680' of trac.sagemath.org:sage into t/30680/finitely_presented_modules_over_the_steenrod_algebra |
f2e59ee | trac 30680: minor final (?) patching |
By the way, regarding this for the profile
method for modules over the Steenrod algebra:
# Avoid returning the zero profile because it triggers a corner case
# in FPModule.resolution().
#
# XXX todo: Fix FPModule_class.resolution().
#
return (1,) if profile == (0,) else profile
I changed it to just return profile
and it worked fine, so other changes must have fixed the problem.
Thank you for all your work on this as well, as well as to the authors for taking the time to write this patch and initial documentation.
I have pushed a few other minor tweaks for the documentation after looking at the final compiled version.
New commits:
a6e43bd | Merge branch 'develop' into u/tscrim/fp_modules_steenrod-30680 |
06c68bb | Merge branch 'u/jhpalmieri/fp_modules_steenrod-30680' of git://trac.sagemath.org/sage into u/tscrim/fp_modules_steenrod-30680 |
e17b853 | Adding Steenrod algebra modules to thematic tutorial index. |
4d77269 | Misc doc tweaks and fixes. |
Changed branch from u/jhpalmieri/fp_modules_steenrod-30680 to u/tscrim/fp_modules_steenrod-30680
Is this a problem or does it just have to be documented?
sage: from sage.modules.fp_graded.module import FPModule
sage: E.<x,y> = ExteriorAlgebra(QQ)
sage: M = FPModule(E, [0, 0], [[x, y]])
sage: M._indices
{(0, 0), (0, 1)}
sage: M.monomial(3)
g[3]
sage: M.monomial(3) in M
True
sage: M.monomial(3) == 0 # This makes it okay, I think.
True
Changed branch from u/tscrim/fp_modules_steenrod-30680 to u/jhpalmieri/fp_modules_steenrod-30680
What do you think about this change?
diff --git a/src/sage/modules/fp_graded/free_element.py b/src/sage/modules/fp_graded/free_element.py
index 8fda1b44115..4da43770af5 100755
--- a/src/sage/modules/fp_graded/free_element.py
+++ b/src/sage/modules/fp_graded/free_element.py
@@ -84,7 +84,7 @@ class FreeGradedModuleElement(IndexedFreeModuleElement):
"""
if order is None:
order = self.parent()._indices
- return super().dense_coefficient_list(order)
+ return [self[i] for i in order]
def degree(self):
This would make the definition of dense_coefficient_list
consistent between element.py
and free_element.py
(and we need methods for both classes because the lift_to_free
method calls dense_coefficient_list
).
Replying to @tscrim:
Question regarding the general graded modules: Why do we have a more interesting
an_element
for theFPModuleHomset
that we override forFreeModuleHomset
(which is just the zero morphism)? (I recognize that previously they were separate classes, but the underlying question still remains why the free one is a more boring element.)
I agree, and I'm going to push a branch that deletes the _an_element_
method for FreeModuleHomset
. It doesn't add anything: users should use an_element
, and if they really want _an_element_
, then the one in parent.pyx
will still be available and will still return the zero morphism.
Branch pushed to git repo; I updated commit sha1. New commits:
3f17e53 | trac 30680: remove some redundant methods |
I removed _an_element_
, also _repr_type
for free morphisms (because that class inherits from the morphism class and they have the same _repr_type
), also minimal_presentation
for SteenrodFPModule
(because it's the same as for FPModule
). I moved some of the doctests from the deleted methods so the tests are still being run.
I also changed dense_coefficient_list
for free module elements, as I proposed in comment:129.
I strongly think we should not put in the monomial
"bug" in the documentation as it just is not checking the input. This is something that comes from the abstract code and was decided not to do this check for speed reasons. Essentially I think of it as garbage-in-garbage-out.
I am not sure I fully agree with the comment:129 change as it could mean if there are improvements in a base class, it will not benefit from it. I am okay with having a different implementation for the FP module since it is not a (finite dimensional) module with a basis. However, I don't mind this change because of the uniformity. The only reason we need for the free module element is because of the ordering issue we came across above.
Everything else is good.
Replying to @tscrim:
I strongly think we should not put in the
monomial
"bug" in the documentation as it just is not checking the input. This is something that comes from the abstract code and was decided not to do this check for speed reasons. Essentially I think of it as garbage-in-garbage-out.
I am concerned that a user will think that M.monomial(0)
will return the 0th generator, leading to an issue that will be a little difficult to debug, especially in this case where M.monomial(3)
prints as g[3]
, not as 0; you have to actually ask whether it's zero to realize what's going on. In other words, users may not realize that it's "garbage-in". So I would prefer to make it explicit.
I am not sure I fully agree with the comment:129 change as it could mean if there are improvements in a base class, it will not benefit from it. I am okay with having a different implementation for the FP module since it is not a (finite dimensional) module with a basis. However, I don't mind this change because of the uniformity. The only reason we need for the free module element is because of the ordering issue we came across above.
The uniformity is the main issue for me. I am worried that some change elsewhere (for example in super().dense_coefficient_list()
) will result in different behavior in these two cases which would lead to subtle problems.
Replying to @jhpalmieri:
Replying to @tscrim:
I strongly think we should not put in the
monomial
"bug" in the documentation as it just is not checking the input. This is something that comes from the abstract code and was decided not to do this check for speed reasons. Essentially I think of it as garbage-in-garbage-out.I am concerned that a user will think that
M.monomial(0)
will return the 0th generator, leading to an issue that will be a little difficult to debug, especially in this case whereM.monomial(3)
prints asg[3]
, not as 0; you have to actually ask whether it's zero to realize what's going on. In other words, users may not realize that it's "garbage-in". So I would prefer to make it explicit.
But we can remove those changes if you want.
Replying to @jhpalmieri:
Replying to @tscrim:
I strongly think we should not put in the
monomial
"bug" in the documentation as it just is not checking the input. This is something that comes from the abstract code and was decided not to do this check for speed reasons. Essentially I think of it as garbage-in-garbage-out.I am concerned that a user will think that
M.monomial(0)
will return the 0th generator, leading to an issue that will be a little difficult to debug, especially in this case whereM.monomial(3)
prints asg[3]
, not as 0; you have to actually ask whether it's zero to realize what's going on. In other words, users may not realize that it's "garbage-in". So I would prefer to make it explicit.
If they haven't read the documentation on this method, they aren't going to read the warning. I think you are also relying on undocumented behavior that could change at any time. Actually, what we should do I think, since we define our own _monomial
function, is do the check and raise an error on invalid input.
I am not sure I fully agree with the comment:129 change as it could mean if there are improvements in a base class, it will not benefit from it. I am okay with having a different implementation for the FP module since it is not a (finite dimensional) module with a basis. However, I don't mind this change because of the uniformity. The only reason we need for the free module element is because of the ordering issue we came across above.
The uniformity is the main issue for me. I am worried that some change elsewhere (for example in
super().dense_coefficient_list()
) will result in different behavior in these two cases which would lead to subtle problems.
Since we are explicitly passing the order
and the method's API specifies a particular behavior for this, any subtle problem is definitely an honest bug in the result of super().dense_coefficient_list()
. This discussion is solidifying something I had thought about introducing: an ABC or mixin class for the two element methods to enforce more consistency between the two. Well, we can easily enough refactor this later if we deem it necessary. We can leave this as-is to suggest that in the code as well.
Branch pushed to git repo; I updated commit sha1. New commits:
7edf174 | trac 30680: test whether index is in indices in FPModule._monomial |
Replying to @tscrim:
Replying to @jhpalmieri:
Replying to @tscrim:
I strongly think we should not put in the
monomial
"bug" in the documentation as it just is not checking the input. This is something that comes from the abstract code and was decided not to do this check for speed reasons. Essentially I think of it as garbage-in-garbage-out.I am concerned that a user will think that
M.monomial(0)
will return the 0th generator, leading to an issue that will be a little difficult to debug, especially in this case whereM.monomial(3)
prints asg[3]
, not as 0; you have to actually ask whether it's zero to realize what's going on. In other words, users may not realize that it's "garbage-in". So I would prefer to make it explicit.If they haven't read the documentation on this method, they aren't going to read the warning. I think you are also relying on undocumented behavior that could change at any time. Actually, what we should do I think, since we define our own
_monomial
function, is do the check and raise an error on invalid input.
I added a check to _monomial
. My earlier point is that once they see the error, then they will look at the documentation and may not still realize the problem, until they remember/realize that "indices" doesn't just mean 0, 1, 2, .... Now it should be better because we will just raise an error.
By the way, I tried a few computations and this check doesn't seem to have slowed things down.
Replying to @jhpalmieri:
I added a check to
_monomial
. My earlier point is that once they see the error, then they will look at the documentation and may not still realize the problem, until they remember/realize that "indices" doesn't just mean 0, 1, 2, .... Now it should be better because we will just raise an error.
Thank you. I think it would have be a little tricky to track the input down to this without knowing what to look for ahead of time.
By the way, I tried a few computations and this check doesn't to have slowed things down.
I am not surprised as this method is not called very often and the check is very quick. This is not the case for other instances in the wild.
I am thinking of setting a positive review now. Any objections?
I think I'm ready. We can tinker more in a follow-up if we see any more issues, plus the list in comment:109.
Thank you for all of your work on this. If #33323 starts looking like it will take a long time, we can simply change the doctests that depend on that.
And there was much rejoicing!
Replying to @jhpalmieri:
And there was much rejoicing!
Indeed!
I am very happy this code finally seems to be in the right place with regards to the rest of Sage. In particular, it's great that FPModules can be created over other algebras than the Steenrod algebra!
I ran some tests today, comparing the performance of the code we submitted for review with the current version: Computing the free resolution of the module M = A/(Sq^12, Sq^7) (A = the mod 2 Steenrod algebra) is 3x faster now! In my example, I computed the length 3 resolution which took ~303 seconds before, and ~106 seconds now, on my laptop. I ran similar tests with other modules, with consistent conclusions.
Thanks a lot, both of you, for your great effort!
Can we now do e.g. FPModules over finitely presented group algebras?
Well, the basic setup in #32505 is for f.p. modules specifically over graded rings, and in parts of the code you want to compute the basis for a particular homogeneous piece, so this relies on the graded ring and the graded module being finite enough that each homogeneous piece is finite-dimensional. It's certainly worth exploring how much the code can be pushed when the graded ring is concentrated in degree zero and is infinite dimensional, but I don't know if anything would work with the current state of things.
One of the nice observations about the Steenrod algebra is that it is a union of finite-dimensional subalgebras, and you can do many useful calculations by restricting to those subalgebras. So maybe everything should in principle work if you have something like an algebra which is filtered by finite-dimensional subalgebras (and if you want to do homological algebra, you probably want each algebra filtration piece to be free over its subalgebra filtration pieces). Some motivated person should look into this.
Documentation doesn't build
Changed branch from u/jhpalmieri/fp_modules_steenrod-30680 to u/tscrim/fp_modules_steenrod-30680
Without this change, I see the same error as reported by the patchbot. I figured my linking to the index.rst
was harmless, but it exposed something that wasn't done properly.
New commits:
43c1bfc | Merge branch 'u/jhpalmieri/fp_modules_steenrod-30680' of git://trac.sagemath.org/sage into u/tscrim/fp_modules_steenrod-30680 |
9bde37e | Small tweak to the doc. |
Changed branch from u/tscrim/fp_modules_steenrod-30680 to u/jhpalmieri/fp_modules_steenrod-30680
This package implements finitely presented modules over the mod p Steenrod algebra. We define classes for such finitely presented modules, their elements, and morphisms between them. Methods are provided for doing some homological algebra, e.g., computing kernels and images of morphisms, and finding free resolutions of modules.
Depends on #32505 Depends on #33275 Depends on #33323
CC: @sverre320 @sagetrac-kvanwoerden @jhpalmieri @tscrim @rrbruner @cnassau
Component: algebra
Keywords: Steenrod algebra, modules, homological algebra
Author: Bob Bruner, Michael Catanzaro, Sverre Lunøe-Nielsen, Koen van Woerden
Branch/Commit:
7035ae5
Reviewer: John Palmieri, Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/30680