Bug fixes for finitely presented graded modules (#32505)

Question

Bug fixes for finitely presented graded modules (#32505)

Closed this issue 4 years ago · 24 comments

jhpalmieri commented 4 years ago

Some bug fixes for the ticket #32505.

Depends on #32505

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

Component: algebra

Author: John Palmieri

Branch/Commit: 29dbd23

Reviewer: Travis Scrimshaw

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

Answer 1 · 2022-02-02T17:54:36.000Z

Branch: u/jhpalmieri/fp-module-bugs

Answer 2 · 2022-02-02T17:55:57.000Z

Commit: e3d4234

Answer 3 · 2022-02-02T17:55:57.000Z

comment:2

Another item that I think needs to be fixed: in the resolution method:

            sage: E.<x,y> = ExteriorAlgebra(QQ)
            sage: triv = FPModule(E, [0], [[x], [y]]) # trivial module
            sage: triv.resolution(3)
            [Free module morphism:
               From: Free graded left module on 1 generator over The exterior algebra of rank 2 over Rational Field
               To:   Finitely presented left module on 1 generator and 2 relations over The exterior algebra of rank 2 over Rational Field
               Defn: g[0] |--> g[0],
             Free module morphism:
               From: Free graded left module on 2 generators over The exterior algebra of rank 2 over Rational Field
               To:   Free graded left module on 1 generator over The exterior algebra of rank 2 over Rational Field
               Defn: g[1, 0] |--> x*g[0]
                     g[1, 1] |--> y*g[0],

The first map here is not a "Free module morphism" because its codomain is not free.

Last 10 new commits:

`72e455e`	`We do not require the base algebra's base ring to be a field.`
`11b3725`	`trac 32505: allow the resolution to be made up of maps between`
`fcce24d`	`Merge branch 'public/modules/free_graded_modules-32505' of git://trac.sagemath.org/sage into public/modules/free_graded_modules-32505`
`2e0518f`	`Using free modules where possible and improved compatibility.`
`6c1ab8d`	`trac 32505: more fixes`
`6cf6d14`	`Merge branch 'public/modules/free_graded_modules-32505' of git://trac.sagemath.org/sage into public/modules/free_graded_modules-32505`
`5074464`	`Some last fixes and removing redundancy.`
`ffe7179`	`trac 32505: replace "PlusInfinity()" with "infinity"`
`a1a9467`	`trac 32505: remove redundant "zero" and "identity" methods`
`e3d4234`	`trac 33275: bug fixes for finitely presented graded modules.`

Answer 4 · 2022-02-02T18:01:57.000Z

Dependencies: #32505

Answer 5 · 2022-02-02T20:53:38.000Z

comment:4

Another view of the problem above:

sage: from sage.modules.fp_graded.module import FPModule
sage: from sage.modules.fp_graded.free_module import FreeGradedModule
sage: E.<x,y> = ExteriorAlgebra(QQ)
sage: triv = FPModule(E, [0], [[x], [y]])
sage: M = FreeGradedModule(E, [0, 1])
sage: type(Hom(M, triv))
<class 'sage.modules.fp_graded.free_homspace.FreeGradedModuleHomspace_with_category_with_equality_by_id'>
sage: type(Hom(triv, M))
<class 'sage.modules.fp_graded.homspace.FPModuleHomspace_with_category_with_equality_by_id'>

It seems to be determining the type of homset from the domain, not paying attention to the codomain.

Answer 6 · 2022-02-02T23:57:34.000Z

comment:5

Replying to @jhpalmieri:

Another view of the problem above:

[snip]

It seems to be determining the type of homset from the domain, not paying attention to the codomain.

I thought about this, and I made a deliberate choice here (one which the code was actually already doing, it just didn't show up explicitly in the doctests). Having it be a free module homomorphism I think is the correct thing to do because we can take advantage of the fact the domain is a free module. I don't think I changed the doc for FreeGradedModuleMorphism, which only says that the codomain must be a graded module. So the output mentioned in comment:2 is correct IMO. For the code as it is now, there is no need to make a distinction based on the codomain AFAICS.

Answer 7 · 2022-02-03T00:07:53.000Z

comment:6

Okay. It looks a little strange, but it does mean that all of the maps in M.resolution() are composable, which is nice (and could be awkward to handle if the 0th map is of a different type than the rest). If something related to this is contributing to issues in #30680, we can revisit it.

I'll mark this as "needs review" now.

Answer 8 · 2022-02-03T01:55:22.000Z

comment:7

Why are you testing degree() in FPModuleElement.coefficients()? Was this meant to be in degree()?

     - ``check`` -- boolean (default: ``True``); if ``True``, check
-      that the morphism is well-defined.
+      that the morphism is well-defined

This is more of an optimization, but it might be better to use self.codomain().linear_combination() in __call__ instead of

        value = sum((c * v for c, v in zip(x.coefficients(), self._values)),
                    self.codomain().zero())

If the FPModule class does not implement it, we should add it for compatibility.

I am also thinking if we should store _values for a FreeMorphism as a dict, then we could use x.monomial_coefficients(copy=False) to only get the non-zero entries. This would also make it order independent. Although then we get into issues of sparse-vs-dense implementations.

Note that coefficients() for the free module now has a different behavior than for CFM and from the category as it contains 0 entries. (Something I missed previously.) I think this should be changed and within the code we should use dense_coefficient_list() (with possibly passing the fixed order where necessary). In particular, I consider this to be a bug.

Answer 9 · 2022-02-03T03:39:00.000Z

Changed commit from e3d4234 to e873dc8

Answer 10 · 2022-02-03T03:39:00.000Z

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

`e873dc8`	`trac 33275: change "coefficients" to "dense_coefficients"`

Answer 11 · 2022-02-03T03:46:14.000Z

comment:9

Replying to @tscrim:

Why are you testing degree() in FPModuleElement.coefficients()? Was this meant to be in degree()?

It was a test of the change in the coefficients method (removing the sorting), but it might make more sense in degree.

     - ``check`` -- boolean (default: ``True``); if ``True``, check
-      that the morphism is well-defined.
+      that the morphism is well-defined

Done, and also got rid of an extra blank line there, too.

This is more of an optimization, but it might be better to use self.codomain().linear_combination() in __call__ instead of
        value = sum((c * v for c, v in zip(x.coefficients(), self._values)),
                    self.codomain().zero())
If the FPModule class does not implement it, we should add it for compatibility.

It's already implemented, so I used it.

I am also thinking if we should store _values for a FreeMorphism as a dict, then we could use x.monomial_coefficients(copy=False) to only get the non-zero entries. This would also make it order independent. Although then we get into issues of sparse-vs-dense implementations.

I haven't done this. Things are working without it, but we could make this change. We could do the same for coefficients for elements.

Note that coefficients() for the free module now has a different behavior than for CFM and from the category as it contains 0 entries. (Something I missed previously.) I think this should be changed and within the code we should use dense_coefficient_list() (with possibly passing the fixed order where necessary). In particular, I consider this to be a bug.

I changed the name to dense_coefficient_list.

Answer 12 · 2022-02-03T03:51:12.000Z

Changed commit from e873dc8 to f9ff665

Answer 13 · 2022-02-03T03:51:12.000Z

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

`f9ff665`	`trac 33275: change "coefficients" to "dense_coefficient_list"`

Answer 14 · 2022-02-03T06:06:11.000Z

Changed commit from f9ff665 to f550b61

Answer 15 · 2022-02-03T06:06:11.000Z

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

`f550b61`	`trac 33275: use linear_combination in a few more places`

Answer 16 · 2022-02-03T08:07:14.000Z

comment:12

Thank you for the changes.

Replying to @jhpalmieri:

Replying to @tscrim:

I am also thinking if we should store _values for a FreeMorphism as a dict, then we could use x.monomial_coefficients(copy=False) to only get the non-zero entries. This would also make it order independent. Although then we get into issues of sparse-vs-dense implementations.

I haven't done this. Things are working without it, but we could make this change. We could do the same for coefficients for elements.

Right now all of the examples are effectively acting like dense modules or are small rank. This would be a more invasive change, so I would postpone it for later unless you have a strong desire for it.

Answer 17 · 2022-02-03T08:12:43.000Z

comment:13

Hopefully the patchbot will come around, but otherwise I am happy with this ticket modulo I would use super() instead of directly calling the category in dense_coefficient_list().

Answer 18 · 2022-02-03T17:09:40.000Z

comment:14

Replying to @tscrim:

Hopefully the patchbot will come around, but otherwise I am happy with this ticket modulo I would use super() instead of directly calling the category in dense_coefficient_list().

I'm probably doing something stupid, but I can't figure out how to do that:

sage: from sage.modules.fp_graded.free_module import FreeGradedModule
sage: A = SteenrodAlgebra()
sage: M = FreeGradedModule(A, (0, 2))
sage: a = M.an_element()
sage: super(FiniteDimensionalModulesWithBasis, a.parent())
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-68-e3772131b543> in <module>
----> 1 super(FiniteDimensionalModulesWithBasis, a.parent())

TypeError: super(type, obj): obj must be an instance or subtype of type

sage: super(FiniteDimensionalModulesWithBasis.element_class, a)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-70-28003f3b5fda> in <module>
----> 1 super(FiniteDimensionalModulesWithBasis.element_class, a)

TypeError: super() argument 1 must be type, not lazy_attribute

I've tried a few other variations, but no luck so far.

Answer 19 · 2022-02-04T01:06:49.000Z

Changed commit from f550b61 to 29dbd23

Answer 20 · 2022-02-04T01:06:49.000Z

Changed branch from u/jhpalmieri/fp-module-bugs to u/tscrim/fp_module_bugs-33275

Answer 21 · 2022-02-04T01:06:49.000Z

Reviewer: Travis Scrimshaw

Answer 22 · 2022-02-04T01:06:49.000Z

comment:16

Here we go. Just use super(). :) If my change is good, then positive review.

New commits:

`29dbd23`	`Last tweaks using super().`

Answer 23 · 2022-02-04T01:27:45.000Z

comment:17

Great, thanks!

Answer 24 · 2022-02-16T23:57:12.000Z

Changed branch from u/tscrim/fp_module_bugs-33275 to 29dbd23