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: sage-8.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 sbrandhorst

  • Branch set to u/sbrandhorst/coercion_pushout_for_fgp_modules

comment:2 Changed 2 years ago by git

  • Commit set to d1580dd206c16cd148d418db24b1c1cf311724bc

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

d1580ddFixed doctest syntax

comment:3 Changed 2 years ago by sbrandhorst

  • Status changed from new to needs_review

comment:4 Changed 2 years ago by dkrenn

  • 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 tscrim

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 sbrandhorst

O.K. but then you have to review it ;).

comment:7 Changed 2 years ago by tscrim

I don't mind reviewing it. I just did so twice on two other (combinatorial-based) algebras.

Last edited 2 years ago by tscrim (previous) (diff)

comment:8 Changed 2 years ago by git

  • Commit changed from d1580dd206c16cd148d418db24b1c1cf311724bc to 85042c0d09cd77ce3f01807a83f5943ac3c6e2e6

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

85042c0Prototype for QuotientModuleFunctor and construction() for FGP_module

comment:9 Changed 2 years ago by sbrandhorst

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:

85042c0Prototype for QuotientModuleFunctor and construction() for FGP_module

comment:10 Changed 2 years ago by git

  • Commit changed from 85042c0d09cd77ce3f01807a83f5943ac3c6e2e6 to 8ee552b7af64203deb11b23aa92be24edac64839

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

8ee552bDocttests.

comment:11 Changed 2 years ago by git

  • Commit changed from 8ee552b7af64203deb11b23aa92be24edac64839 to b249f0c3c8b228fcbd1eefb8b3bb7750b02bd72a

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

b249f0cMore Doctests

comment:12 Changed 2 years ago by sbrandhorst

  • Status changed from needs_work to needs_review

comment:13 Changed 2 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch changed from u/sbrandhorst/coercion_pushout_for_fgp_modules to public/modules/pushout_fgp_modules-23967
  • 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:

68a4541Make the functor behave like QuotientFunctor.

comment:14 follow-up: Changed 2 years ago by dkrenn

  • Authors changed from Travis Scrimshaw to Daniel Krenn, Travis Scrimshaw

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 follow-up: Changed 2 years ago by sbrandhorst

  • Authors changed from Daniel Krenn, Travis Scrimshaw to Simon Brandhorst, Daniel Krenn, Travis Scrimshaw

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?

Last edited 2 years ago by sbrandhorst (previous) (diff)

comment:16 follow-up: Changed 2 years ago by 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. 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 ; follow-up: Changed 2 years ago by tscrim

Replying to dkrenn:

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.

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 ; follow-up: Changed 2 years ago by dkrenn

Replying to tscrim:

Replying to dkrenn:

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.

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 tscrim

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 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?

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 tscrim

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 ; follow-up: Changed 2 years ago by tscrim

Replying to dkrenn:

Replying to tscrim:

Replying to dkrenn:

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.

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 sage-devel 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 dkrenn

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 sage-devel 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 git

  • Commit changed from 68a45417c632bdce1e277e8a8d91de4a4c49e72a to ae67c45604685d97125acdbba7050a6acf06d64d

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

4970aadChanged some doctests.
61dde41fixed doctests
ae67c45Moved to sage/modules/module_functors.py

comment:24 Changed 2 years ago by git

  • Commit changed from ae67c45604685d97125acdbba7050a6acf06d64d to a9380c93606e5f80179b88a9d68389cb1aee61a8

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

a9380c9Fixed imports and doctests. Removed QuotientFunctor from fgp_module

comment:25 Changed 2 years ago by sbrandhorst

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 git

  • Commit changed from a9380c93606e5f80179b88a9d68389cb1aee61a8 to 020f371d3b8d72fa30a1e42e281e2961b2e6ee3f

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

020f371A little polishing.

comment:27 Changed 2 years ago by tscrim

  • Authors changed from Simon Brandhorst, Daniel Krenn, Travis Scrimshaw to Simon Brandhorst Travis Scrimshaw
  • 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 tscrim

  • Authors changed from Simon Brandhorst Travis Scrimshaw to Simon Brandhorst, Travis Scrimshaw

comment:29 Changed 2 years ago by sbrandhorst

  • Status changed from needs_review to positive_review
sage -t --long --warn-long 48.3 src/sage/modules/module_functors.py
    [57 tests, 0.72 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------


sage -t --long --warn-long 48.3 src/sage/modules/fg_pid/fgp_module.py
    [349 tests, 3.43 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

Positive review.

comment:30 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:31 Changed 2 years ago by sbrandhorst

merge conflict with what. How do I find it?

comment:32 Changed 2 years ago by git

  • Commit changed from 020f371d3b8d72fa30a1e42e281e2961b2e6ee3f to f7b901b014e3e4ffce79e85671c3adf60241b05c

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

f7b901bMerge branch 'develop' into t/23967/public/modules/pushout_fgp_modules-23967

comment:33 Changed 2 years ago by tscrim

I believe it is with the next upcoming beta version.

comment:34 Changed 2 years ago by sbrandhorst

Yep. It would be nice to know the ticket :)

comment:35 Changed 2 years ago by git

  • Commit changed from f7b901b014e3e4ffce79e85671c3adf60241b05c to 2865ddd0c92f7d84671145e862000f0685be3b9d

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

2865dddMerge branch 'public/modules/pushout_fgp_modules-23967' of git://trac.sagemath.org/sage into public/modules/pushout_fgp_modules-23967

comment:36 Changed 2 years ago by tscrim

  • 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 vbraun

  • Branch changed from public/modules/pushout_fgp_modules-23967 to 2865ddd0c92f7d84671145e862000f0685be3b9d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.