Opened 2 years ago
Closed 2 years ago
#23967 closed enhancement (fixed)
Coercion pushout for FGP_modules
Reported by:  sbrandhorst  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Simon Brandhorst, Travis Scrimshaw  Reviewers:  Travis Scrimshaw, Simon Brandhorst, Daniel Krenn 
Report Upstream:  N/A  Work issues:  
Branch:  2865ddd (Commits)  Commit:  2865ddd0c92f7d84671145e862000f0685be3b9d 
Dependencies:  Stopgaps: 
Description
The following should be possible:
sage: A = ZZ^2sage: S1 = B.submodule(B.gens()[:1]) sage: S1 = B.submodule(B.gens()[:1]) sage: S2 = B.submodule(B.gens()[1:]) sage: s1 = S1.gens()[0]; s2 = S2.gens()[0] sage: s1+s2 Traceback (most recent call last) ... TypeError: unsupported operand parent(s) for +: 'Finitely generated module V/W over Integer Ring with invariants (2)' and 'Finitely generated module V/W over Integer Ring with invariants (2)'
Let V1,V2,W1,W2 be modules in the same ambient space with V1 containing W1 and V2 containing W2
Let a in V1/W1 and b in V2/W2. Their sum a + b is well defined as an element of (V1+V2)/(W1+W2)
Change History (37)
comment:1 Changed 2 years ago by
 Branch set to u/sbrandhorst/coercion_pushout_for_fgp_modules
comment:2 Changed 2 years ago by
 Commit set to d1580dd206c16cd148d418db24b1c1cf311724bc
comment:3 Changed 2 years ago by
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Status changed from needs_review to needs_work
I think using _pushout_
here is kind of a hack. The correct way to implement it (so that it also works in more general situations) is to derive a class from ConstructionFunctor
, implement the functionality of merge
(similar to what is done now in _pushout_
and return this construction functor when calling .construction()
.
comment:5 Changed 2 years ago by
I would not call it a hack, but it is not the typical way done in Sage as it gives less information (nor is it as flexible I believe). So I'm +1 on using ConstructionFunctor
.
comment:6 Changed 2 years ago by
O.K. but then you have to review it ;).
comment:7 Changed 2 years ago by
I don't mind reviewing it. I just did so twice on two other (combinatorialbased) algebras.
comment:8 Changed 2 years ago by
 Commit changed from d1580dd206c16cd148d418db24b1c1cf311724bc to 85042c0d09cd77ce3f01807a83f5943ac3c6e2e6
Branch pushed to git repo; I updated commit sha1. New commits:
85042c0  Prototype for QuotientModuleFunctor and construction() for FGP_module

comment:9 Changed 2 years ago by
Have fun and thank you. I am going to bed now. Some doctests are missing etc. and I do not know what rank = ?
New commits:
85042c0  Prototype for QuotientModuleFunctor and construction() for FGP_module

comment:10 Changed 2 years ago by
 Commit changed from 85042c0d09cd77ce3f01807a83f5943ac3c6e2e6 to 8ee552b7af64203deb11b23aa92be24edac64839
Branch pushed to git repo; I updated commit sha1. New commits:
8ee552b  Docttests.

comment:11 Changed 2 years ago by
 Commit changed from 8ee552b7af64203deb11b23aa92be24edac64839 to b249f0c3c8b228fcbd1eefb8b3bb7750b02bd72a
Branch pushed to git repo; I updated commit sha1. New commits:
b249f0c  More Doctests

comment:12 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:13 Changed 2 years ago by
 Branch changed from u/sbrandhorst/coercion_pushout_for_fgp_modules to public/modules/pushout_fgp_modules23967
 Commit changed from b249f0c3c8b228fcbd1eefb8b3bb7750b02bd72a to 68a45417c632bdce1e277e8a8d91de4a4c49e72a
 Reviewers set to Travis Scrimshaw
I did a fairly significant rewrite to make the implementation closer to QuotientFunctor
. Specifically, the quotients are determined by the relations module W
, so that is all that should be used to define the functor. I also moved the functor out of the (IMO) overcrowded categories/pushout.py
for better code localization.
If my changes are good, then positive review after you set your name in both the authors and reviewers section.
New commits:
68a4541  Make the functor behave like QuotientFunctor.

comment:14 followup: ↓ 17 Changed 2 years ago by
LGTM, except that I personally prefer to replace self
by something more descriptive, e.g. in
The construction functor and ambient module for ``self``.
replace self
by finitely generated module over a PID
.
There are two successful patchbot results at the top at the moment, however previously some (more than one patchbot) failed in src/sage/interfaces/qepcad.py
. Any ideas why?
comment:15 followup: ↓ 19 Changed 2 years ago by
I have tried to make it behave like a subspace functor.
In future module constructions can be more complicated like
VectorFunctor
, direct, sum, base extension, some quotient, and a tensor product.
So the functor might have to deal with all kinds of other functors.
Why do you fix the base_ring
? The vector functor and subspace functor do not.
And also the quotient functor does not fix a ring.
With your method, construction of QQ / ZZ
is fine.
base extensions when more general pids than ZZ
are finally allowed do not work well with the functor. We could extract the basering as basering of the input. Or/and make it an optional argument of the call?
comment:16 followup: ↓ 20 Changed 2 years ago by
1 for moving it into fgp modules. The functor should not assume that the input is a pid. Quotients of modules make perfect sense over general rings not just PIDS the functor should not care about the implementation. But you are right that pushout is somewhat overcrowded. One could create a file in the module folder and put all module functors there?
comment:17 in reply to: ↑ 14 ; followup: ↓ 18 Changed 2 years ago by
Replying to dkrenn:
LGTM, except that I personally prefer to replace
self
by something more descriptive, e.g. inThe construction functor and ambient module for ``self``.replace
self
byfinitely generated module over a PID
.
I am very strongly in support of self
in docstrings as the rest is too verbose and does not behave well with subclasses.
There are two successful patchbot results at the top at the moment, however previously some (more than one patchbot) failed in
src/sage/interfaces/qepcad.py
. Any ideas why?
This is unrelated.
comment:18 in reply to: ↑ 17 ; followup: ↓ 21 Changed 2 years ago by
Replying to tscrim:
Replying to dkrenn:
LGTM, except that I personally prefer to replace
self
by something more descriptive, e.g. inThe construction functor and ambient module for ``self``.replace
self
byfinitely generated module over a PID
.I am very strongly in support of
self
in docstrings as the rest is too verbose and does not behave well with subclasses.
Ok.
comment:19 in reply to: ↑ 15 Changed 2 years ago by
Replying to sbrandhorst:
I have tried to make it behave like a subspace functor. In future module constructions can be more complicated like
VectorFunctor
, direct, sum, base extension, some quotient, and a tensor product. So the functor might have to deal with all kinds of other functors.
That has no bearing on this functor's implementation.
Why do you fix the
base_ring
? The vector functor and subspace functor do not. And also the quotient functor does not fix a ring.
QuotientFunctor
is Rings
to Rings
, so there is no base rings involved with the categories. In this case, we are doing a quotient by a module that has a specific base ring R
. So it makes sense to be a functor from R
mod to R
mod. For VectorFunctor
, there is not a specific way to specify the base ring. However, if you feel strongly that we should have larger categories for the domain and codomain, then we can change it.
With your method, construction of
QQ / ZZ
is fine. base extensions when more general pids thanZZ
are finally allowed do not work well with the functor. We could extract the basering as basering of the input. Or/and make it an optional argument of the call?
I'm not sure I understand what you're proposing. I do not agree that extension of scalars is a part of this functor. I feel that should be an entirely different functor.
comment:20 in reply to: ↑ 16 Changed 2 years ago by
Replying to sbrandhorst:
1 for moving it into fgp modules. The functor should not assume that the input is a pid. Quotients of modules make perfect sense over general rings not just PIDS the functor should not care about the implementation.
I agree that it could be used more generally. However, where else is it (planned to be) used? It is easy to move it later if the functor is to be used elsewhere, but good code locality (both within files and folders) makes it easier to maintain.
But you are right that pushout is somewhat overcrowded. One could create a file in the module folder and put all module functors there?
I would not oppose this.
comment:21 in reply to: ↑ 18 ; followup: ↓ 22 Changed 2 years ago by
Replying to dkrenn:
Replying to tscrim:
Replying to dkrenn:
LGTM, except that I personally prefer to replace
self
by something more descriptive, e.g. inThe construction functor and ambient module for ``self``.replace
self
byfinitely generated module over a PID
.I am very strongly in support of
self
in docstrings as the rest is too verbose and does not behave well with subclasses.Ok.
However, there has been a discussion(s?) on sagedevel about this and I am in the minority. So if you want to change them, you can and I will not object. However, on my doc changes on files where there is a mix (and nobody has objected), I will be doing the above.
comment:22 in reply to: ↑ 21 Changed 2 years ago by
Replying to tscrim:
Replying to dkrenn:
I am very strongly in support of
self
in docstrings as the rest is too verbose and does not behave well with subclasses.Ok.
However, there has been a discussion(s?) on sagedevel about this and I am in the minority. So if you want to change them, you can and I will not object. However, on my doc changes on files where there is a mix (and nobody has objected), I will be doing the above.
Let's keep it (as both versions are ok; thus the author should have preference).
comment:23 Changed 2 years ago by
 Commit changed from 68a45417c632bdce1e277e8a8d91de4a4c49e72a to ae67c45604685d97125acdbba7050a6acf06d64d
comment:24 Changed 2 years ago by
 Commit changed from ae67c45604685d97125acdbba7050a6acf06d64d to a9380c93606e5f80179b88a9d68389cb1aee61a8
Branch pushed to git repo; I updated commit sha1. New commits:
a9380c9  Fixed imports and doctests. Removed QuotientFunctor from fgp_module

comment:25 Changed 2 years ago by
O.K. tests pass now. If you are happy with the changes, you can set it to positive review after the patchbot is O.K. as well.
comment:26 Changed 2 years ago by
 Commit changed from a9380c93606e5f80179b88a9d68389cb1aee61a8 to 020f371d3b8d72fa30a1e42e281e2961b2e6ee3f
Branch pushed to git repo; I updated commit sha1. New commits:
020f371  A little polishing.

comment:27 Changed 2 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Simon Brandhorst, Daniel Krenn
I made a few essentially trivial changes. I added one extra test of merge
, and everything (still) passes:
sage t src/sage/modules/module_functors.py [57 tests, 0.44 s]  All tests passed! 
So please set to a positive review if this also passes for you.
comment:28 Changed 2 years ago by
comment:29 Changed 2 years ago by
 Status changed from needs_review to positive_review
sage t long warnlong 48.3 src/sage/modules/module_functors.py [57 tests, 0.72 s]  All tests passed!  sage t long warnlong 48.3 src/sage/modules/fg_pid/fgp_module.py [349 tests, 3.43 s]  All tests passed! 
Positive review.
comment:31 Changed 2 years ago by
merge conflict with what. How do I find it?
comment:32 Changed 2 years ago by
 Commit changed from 020f371d3b8d72fa30a1e42e281e2961b2e6ee3f to f7b901b014e3e4ffce79e85671c3adf60241b05c
Branch pushed to git repo; I updated commit sha1. New commits:
f7b901b  Merge branch 'develop' into t/23967/public/modules/pushout_fgp_modules23967

comment:33 Changed 2 years ago by
I believe it is with the next upcoming beta version.
comment:34 Changed 2 years ago by
Yep. It would be nice to know the ticket :)
comment:35 Changed 2 years ago by
 Commit changed from f7b901b014e3e4ffce79e85671c3adf60241b05c to 2865ddd0c92f7d84671145e862000f0685be3b9d
Branch pushed to git repo; I updated commit sha1. New commits:
2865ddd  Merge branch 'public/modules/pushout_fgp_modules23967' of git://trac.sagemath.org/sage into public/modules/pushout_fgp_modules23967

comment:36 Changed 2 years ago by
 Status changed from needs_work to positive_review
I didn't get a merge conflict with 8.1.beta9
, but I pushed the branch just to be sure.
comment:37 Changed 2 years ago by
 Branch changed from public/modules/pushout_fgp_modules23967 to 2865ddd0c92f7d84671145e862000f0685be3b9d
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Fixed doctest syntax