Opened 10 months ago

Closed 7 months ago

#15801 closed enhancement (fixed)

Categories over a base ring category

Reported by: nthiery Owned by:
Priority: major Milestone: sage-6.3
Component: categories Keywords:
Cc: sage-combinat, vbraun, SimonKing, darij Merged in:
Authors: Nicolas M. Thiéry Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 15658bd (Commits) Commit: 15658bd13f22b874423f62b8e6e6fb4d4f7b56ec
Dependencies: #10963, #16275 Stopgaps:

Description (last modified by tscrim)

Currently, to construct a category of algebras, we need to specify the
base ring:

    sage: Algebras(QQ)

A side effect is that the construction of e.g. GF(p)['x'] for a
lot of p's in number theoretic calculations triggers the
construction of many parallel hierarchies of categories.

This is a waste because in most situations only the category of the
base ring is relevant. In particular, the generic code provided to
parents/elements/... only depends on the latter.

The purpose of this ticket is to allow for only specifying the
category of the base ring, as in:

    sage: Algebras(Fields())

with

    sage: Algebras(QQ)

constructing a subcategory of the former. Then go trough the code,
decide in each case whether we want to use Algebras(QQ) or
Algebras(Fields()), and update the doctests accordingly. In most
case, the latter idiom will be preferable, unless we need some
operation on the category itself.

This would fix the regression noted on #15792 and made worse by
#10963.

Further features for this ticket or follow-up tickets would be to:

  • make Modules(...) into a covariant functorial constructions. This would give a proper framework for the feature Modules(QQ) -> VectorSpaces(QQ) instead of having to tackle it with a special hack.
  • implement a PolynomialRings(...) covariant functorial construction.

Change History (73)

comment:1 Changed 10 months ago by SimonKing

As commented on #10963, I see some questions to be addressed.

If I understand correctly, we sometimes do want to specify the base ring/field, and Volker says that this should be by a "mixin" category (lets call it FixedBaseRing(...)) that simply is joined with the non-specific category, say, Algebras(Fields()).

First complication: We not only have base rings, but (for bimodules) left and right base rings. Should this be reflected in the "FixedBaseBla?" categories as well?

And another complication: We want to have categories expressing the fact that a field (not just a ring) is acting on the objects, and then want to compute the join. Is there an easy programmatic way to let an error be raised when someone tries to compute the join "Algebras(Fields()) & FixedBaseRing?(ZZ)"?

comment:2 Changed 10 months ago by nthiery

Replying to SimonKing:

So, the strategy would be this: The current "Category over base ring" should accept as input no a ring, but a subcategory of the category of rings.

Or more precisely, it would accept both kinds of input.

A polynomial ring should be initialised in the category of "commutative algebras over Category of quotient fields" (say), and should of course have its own reference to the base ring (say, the rational field).

Yup. And later in the category of "polynomial rings over a quotient field."

Since the parent and element classes only depend on the category of the base ring (by the current implementation), a category refinement would probably not be needed.

Yes.

But *if* needed, it would amount to take the join with the category "FixedBaseField(QQ), perhaps with super-categories "FixedBaseRing(QQ)".

Yes, though I would write it as taking the join with Algebras(QQ).

Additional technical problem: We not only have base rings, but (for bimodules) left and right base rings. Should this be reflected in the "FixedBaseBla" categories as well?

We will want to accept both:

    Bimodules(Fields(), Fields())

and

    Bimodules(QQ, QQ)

In a first step, I would not bother about mixed calls likes Bimodules(QQ, Fields)/

And another complication: We want to have categories expressing the fact that a field (not just a ring) is acting on the objects, and then want to compute the join. Is there an easy programmatic way to let an error be raised when someone tries to compute the join "Algebras(Fields()) & FixedBaseRing(ZZ)"?

I don't see a trivial way to do it at this point. It certainly would
be a nice feature to bark upon such constructions, but not having it
is not really worst than the current situation where we have:

sage: Algebras(QQ) & Algebras(GF(3))
Join of Category of algebras over Finite Field of size 3 and Category of algebras over Rational Field

So this issue should not prevent us to proceed even if we don't find a good way out.

comment:3 follow-up: Changed 10 months ago by SimonKing

OK, that's the other possibility: Rather than having a separate mix-in category FixedBaseRing(...), we would allow both Algebras(Fields()) and Algebras(QQ).

I have already done some experiments in that direction. We certainly want Algebras(R.category()) among the super-categories of Algebras(R). But if C is a category, do we want that Algebras(D) is among the super-categories of Algebras(C), for all D in C.super_categories()?

When I tried it yesterday, Nicolas' bounded C3 algorithm barked at me. Reason (if I recall correctly, I can't use my laptop right now): We want that Algebras(QQ) and Algebras(QQ.category() have the same parent and element classes, hence, _make_named_class_key returns the same, and thus they also share the cmp-key used in the C3 algorithm. But the C3 algorithm expects that the items on the given lists (here: the list of all super categories) have distinct keys.

One possible approach to solve that problem: The strict super-categories of a category with fixed base ring should all be categories with unspecified base rings. Hence, Algebras(R) should not have the super-categories Rings() and Modules(R), but Algebras(R.category()).

comment:4 in reply to: ↑ 3 ; follow-up: Changed 10 months ago by nthiery

Replying to SimonKing:

OK, that's the other possibility: Rather than having a separate mix-in category FixedBaseRing(...), we would allow both Algebras(Fields()) and Algebras(QQ).

I have already done some experiments in that direction. We certainly want Algebras(R.category()) among the super-categories of Algebras(R). But if C is a category, do we want that Algebras(D) is among the super-categories of Algebras(C), for all D in C.super_categories()?

When I tried it yesterday, Nicolas' bounded C3 algorithm barked at me. Reason (if I recall correctly, I can't use my laptop right now): We want that Algebras(QQ) and Algebras(QQ.category() have the same parent and element classes, hence, _make_named_class_key returns the same, and thus they also share the cmp-key used in the C3 algorithm. But the C3 algorithm expects that the items on the given lists (here: the list of all super categories) have distinct keys.

One possible approach to solve that problem: The strict super-categories of a category with fixed base ring should all be categories with unspecified base rings. Hence, Algebras(R) should not have the super-categories Rings() and Modules(R), but Algebras(R.category()).

This sounds reasonable. A variant would be, for a category over base
ring C, to set C(R).parent_class explicitly to
C(R.category()).parent_class, bypassing the
_make_named_class_key business since it won't really be needed
anymore anyway (and similarly for the element class and so on).

I'd say that we certainly want Algebras(R) to be a subcategory
of Modules(R), as tested by is_subcategory. But we could
imagine doing like for joins, and *not* put Modules(R) in
Algebras(R).<all_>super_categories().

In fact, maybe Algebras(R) should really be a join category:
that of Algebras(Fields()) & Modules(R).

<ramble> Here Modules(R) would play a role similar to the category
with axiom C().A() associated to the category C defining
the axiom A. We would not have the option to define the analogue
of a category with axioms for a subcategory, but at this point I
don't foresee a use for it either.</ramble>

Cheers,

Nicolas

comment:5 in reply to: ↑ 4 ; follow-up: Changed 10 months ago by SimonKing

  • Dependencies set to 10963

We want to build on top of #10963, right?

Replying to nthiery:

One possible approach to solve that problem: The strict super-categories of a category with fixed base ring should all be categories with unspecified base rings. Hence, Algebras(R) should not have the super-categories Rings() and Modules(R), but Algebras(R.category()).

This sounds reasonable. A variant would be, for a category over base
ring C, to set C(R).parent_class explicitly to
C(R.category()).parent_class, bypassing the
_make_named_class_key business since it won't really be needed
anymore anyway (and similarly for the element class and so on).

I think _make_named_class_key would still be make sense: We need to distinguish the element and parent classes of Algebras(Fields()) from Algebras(Rings()), but probably not from Algebras(QuotientFields()). And in particular (when following your suggestion below) we would still want that Modules(GF(5)) and Modules(GF(7)) have the same parent and element classes.

I'd say that we certainly want Algebras(R) to be a subcategory
of Modules(R), as tested by is_subcategory. But we could
imagine doing like for joins, and *not* put Modules(R) in
Algebras(R).<all_>super_categories().

Yes, but then one needs a category FixedBase(R) to be joined with! And I think that's not so nice, as pointed out in comment:1. Or so I thought. But:

In fact, maybe Algebras(R) should really be a join category:
that of Algebras(Fields()) & Modules(R).

Hmmm. Yes, in that case one wouldn't need an ugly FixedBase(R) category...

<ramble> Here Modules(R) would play a role similar to the category
with axiom C().A() associated to the category C defining
the axiom A. We would not have the option to define the analogue
of a category with axioms for a subcategory, but at this point I
don't foresee a use for it either.</ramble>

Well, that's what I commented on at #10963: I was playing with the idea of an axiom WithFixedBase, accepting an argument, but Volker has cut the argument short ;-).

comment:6 Changed 10 months ago by SimonKing

  • Dependencies changed from 10963 to #10963

comment:7 in reply to: ↑ 5 Changed 10 months ago by nthiery

Replying to SimonKing:

We want to build on top of #10963, right?

Definitely.

Replying to nthiery:
I think _make_named_class_key would still be make sense: We need to distinguish the element and parent classes of Algebras(Fields()) from Algebras(Rings()), but probably not from Algebras(QuotientFields()).

That would be for free if we make Algebras a functorial construction:
if a category, say QuotientFields(), says nothing about algebras
over itself, then Algebras(QuotientFields()) will actually
return Algebras(Fields()).

And in particular (when following your suggestion below) we would still want that Modules(GF(5)) and Modules(GF(7)) have the same parent and element classes.

Right, if we go the join way, we will need to handle the base case of
Modules and have a make_named_class_key mechanism there.

comment:8 Changed 10 months ago by nbruin

Just a little note. In http://trac.sagemath.org/ticket/10963#comment:261 it was noted that base categories are referenced strongly by a global cache, and hence are prone to cause "memory leaks". This was rebutted there by noting that the number of categories created is uniformly bounded, so the memory use is manageable. This is clearly not true for these parametrized "categories with fixed base".

The work on this ticket will improve the situation, because categories parametrized by elements from an infinite set will be less often created, but it's worthwhile to try and think of what to do if they do get created. Perhaps we should really try and avoid them completely? Certainly the infrastructure requirements to support them is radically different, because such parametrized categories need to be deallocatable (I'm impressed that Firefox's spell checker accepts that word and not Firefox).

comment:9 follow-up: Changed 9 months ago by nbruin

A simple test that detects some of the problems #10963 presently causes:

while True: k=random_matrix(Rationals(),30,20).echelon_form()

On 6.2beta4 this happily runs with constant memory footprint and about constant time per iteration.

With #10963 applied, it gradually eats memory. The reason is that "integers mod p" and their categories pile up in memory. The iterations also take about twice as long and they slow down over time (as one would expect with memory use increasing).

(see #13925 for why the choice of algorithm might not be so sensible, but it's a useful test for our purposes here)

comment:10 Changed 8 months ago by nthiery

Hi!

I started to work on this. We can now do:

    sage: Algebras(Fields())
    Category of algebras over Category of fields

(TODO: would be nicer as Category of algebras over fields)

and by default, polynomial rings are created in such categories:

    sage: QQ['x'].category()
    Join of Category of euclidean domains and Category of commutative algebras over Category of quotient fields

All tests pass in the sage/rings/polynomial.

Here are the new timing for Simon's benchmark about creating lots of
polynomial rings:

With Sage 5.0.1:

sage: %time test()
CPU times: user 7.38 s, sys: 0.08 s, total: 7.46 s
Wall time: 7.48 s

With develop (6.2.beta7):

sage: %time test()
CPU times: user 9.31 s, sys: 125 ms, total: 9.44 s
Wall time: 9.47 s

With #10963:

sage: %time test()
CPU times: user 17.6 s, sys: 288 ms, total: 17.9 s
Wall time: 17.8 s

With #10963 and this ticket:

sage: def test():
....:     for p in prime_range(10000):
....:         P = GF(p)['t','x','z']
sage: %time test()
CPU times: user 2.99 s, sys: 127 ms, total: 3.11 s
Wall time: 3.03 s

Sounds like the time-performance issue for polynomials is indeed fixed for polynomials :-)

Now, on to matrices!

comment:11 Changed 8 months ago by nthiery

  • Branch set to refs/heads/public/categories/over-a-base-ring-category-15801

comment:12 Changed 8 months ago by nthiery

  • Branch changed from refs/heads/public/categories/over-a-base-ring-category-15801 to public/categories/over-a-base-ring-category-15801

comment:13 in reply to: ↑ 9 Changed 8 months ago by nthiery

  • Commit set to 3a63847b3e313d2e4eb0261f156e7d0bb7bf7a9c

Replying to nbruin:

A simple test that detects some of the problems #10963 presently causes:

while True: k=random_matrix(Rationals(),30,20).echelon_form()

On 6.2beta4 this happily runs with constant memory footprint and about constant time per iteration.

With #10963 applied, it gradually eats memory. The reason is that "integers mod p" and their categories pile up in memory. The iterations also take about twice as long and they slow down over time (as one would expect with memory use increasing).

(see #13925 for why the choice of algorithm might not be so sensible, but it's a useful test for our purposes here)

For the record: I just checked, and with the current version of this
ticket on top of #10963, this runs with constant memory footprint and
about constant time per iteration.

Cheers,

Nicolas


Last 10 new commits:

1b93d6eMerge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
28b39a2Merge branch 'develop' into public/ticket/10963-doc-distributive
70972dbMerge branch 'develop' into public/ticket/10963-doc-distributive
e1110daTrac 10963: added little note in Category._super_categories
ad718deTrac 10963: added mathematical definitions to the documentation of a bunch of axioms
d24a3d5Trac 15801: enable categories over a base ring category and use them in polynomials
6bf3893Trac 15801: improved repr for categories over base ring categories and fixed doctest
7db4ef0Trac 10963: Fixed repr for the symmetric group algebra by improving a bit the generic repr methods in the categories
1116ef5Merge branch 'categories/axioms-10963' into categories/over_a_base_category-15801
3a63847Trac 15801: updated doctest

comment:14 Changed 8 months ago by nthiery

Replying to nbruin:

There are some performance and leaking issues with the proposed code here. They
have been mentioned before, but they got swamped in more theoretical design
discussions. Consider for instance with sage 6.2.beta4:

...

As you can see, much slower execution and much leakier behaviour (even without
the patch this kind of stuff is very liable to leak but as you can see, this
patch makes it much worse). Note that we have to sort types by their string
representation, because for each of these ..._with_category types, the 5000 instances are in fact unique types with identical print names. So the memory load is even higher (types are fairly heavy data structures). As you can see, categories are eternal (they get cached) and since they store a reference to their base, they nail the base ring in memory too. We'll have to merge this ticket together with a ticket that moves base ring references out of the categories, since the constructions introduced in this ticket make the problem urgent.

For the record, here is what I currently get on my machine:

With this ticket applied on top of #10963:

sage: import gc
sage: from collections import Counter
sage: before={id(c) for c in gc.get_objects()}
sage: %time for n in [1..5000]: x=(matrix(Integers(n),2,2,1))
CPU times: user 3.83 s, sys: 41.2 ms, total: 3.87 s
Wall time: 3.84 s
sage: sage: gc.collect()
17047
sage: sage: gc.collect()
864
sage: sage: after=[c for c in gc.get_objects() if id(c) not in before]
sage: sage: [c for c in Counter([str(type(a)) for a in after]).items() if c[1] > 4000]
[]

Same, without systematic full category initialization for matrices:

CPU times: user 3.49 s, sys: 52.8 ms, total: 3.54 s
Wall time: 3.49 s
sage: sage: sage: sage: gc.collect()
18836
sage: sage: sage: sage: gc.collect()
1344

With vanilla 6.2.beta6 (no systematic full category initialization for matrices):

CPU times: user 2.89 s, sys: 33.6 ms, total: 2.92 s
Wall time: 2.88 s
sage: sage: sage: gc.collect()
5032
sage: sage: sage: gc.collect()
704

Hence #15801 as is seems to be going a long way toward fixing this
issue. It's still non negligibly slower than develop on this very
tight loop (and apparently more objects get
allocated/deallocated). But in exchange we are back to systematic full
category initialization which I believe is much nicer to the user.

What do you think?

On to the elliptic curve benchmarks.

Cheers,

Nicolas

comment:15 Changed 8 months ago by nthiery

On the benchmark from #11900, I get significantly the same timing with develop and #10963+#15801:

sage: E = J0(46).endomorphism_ring()
sage: %time g = E.gens()
CPU times: user 3.63 s, sys: 94.9 ms, total: 3.73 s
Wall time: 3.90 s    #develop     
Wall time: 3.98 s    #10963
Wall time: 3.86 s    #10963+#15801   

This, despite the additional full category initialization.

Last edited 8 months ago by nthiery (previous) (diff)

comment:16 Changed 8 months ago by git

  • Commit changed from 3a63847b3e313d2e4eb0261f156e7d0bb7bf7a9c to 2121f513a9f7694c6a4c13b9803ed5df9954304b

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

f44661fTrac 15801: added repr in HeckeModule_free_module (the generic one in modules with basis can't handle it anymore)
545765aMerge branch 'categories/axioms-10963' into categories/over_a_base_category-15801
961bfb6Trac 15801: reinstated test in vector spaces after some mess up in the #10963 branch; updated some doctests
a051864Trac 15801: deprecates MatrixSpace.full_category_initialisation which is now unneeded
42ab3cdTrac 15801: update thematic_tutorials/coercion_and_categories.rst w.r.t the full initialization of matrix spaces
9cc7ef4Trac 15801: little fix in polynomial_default_category, and update doctests involving categories of polynomial rings
3d2ec28Trac 15801: fixed recent missing import
cf847f8Trac 15801: use a group algebra rather than a polynomial algebra in an example to keep it interesting
fa60747Trac 15801: fixed typo in doctest
2121f51Trac 15801: improved subcategory and containment tests for categories over a base ring category

comment:17 Changed 8 months ago by nthiery

Up to merging in #15919, it's likely that all long test will pass. Running them now.

Last edited 8 months ago by nthiery (previous) (diff)

comment:18 Changed 8 months ago by git

  • Commit changed from 2121f513a9f7694c6a4c13b9803ed5df9954304b to 12afbe46ebafe56cf74661bb6d5ea81af583ba83

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

12afbe4Trac 15801: fixed ReST typo

comment:19 follow-up: Changed 8 months ago by tscrim

  • Description modified (diff)

So Algebras(Rings()) would cover any algebra (well...any assoc & unital algebra)?

Also I think we should not loose support for having the category of algebras over a fixed base ring. For example, given two representations V, W of a group algebra RG over R, the morphisms from V to W as RG-modules is (significantly) smaller than the morphisms as R-modules. Currently our set of morphisms depends on the category, which if we keep the current setup, would give that Hom(V, W, Modules(RG)) is Hom(V, W, Modules(R)) (well, maybe only ==).

Actually...RG would be the category of group algebras whereas R would be in the category of rings, unless R was also the group algebra of some other group (okay, it's very contrived, but possible). My point is that just passing the category into Modules doesn't always tell us the full structure (of the homset), and is there some way we can work around this?

comment:20 in reply to: ↑ 19 Changed 8 months ago by nthiery

Replying to tscrim:

So Algebras(Rings()) would cover any algebra (well...any assoc & unital algebra)?

Yup. And at some point we will want weaker things like Algebras(SemiRings()) and so on.

Also I think we should not loose support for having the category of algebras over a fixed base ring.

I agree with that, though more if we want to manipulate the category
"mathematically". From a code perspective point of view, since #11900,
the provided abstract classes only depends on the category of the base
ring, and this has not been a limitation so far.

For example, given two representations V, W of a group algebra RG over R, the morphisms from V to W as RG-modules is (significantly) smaller than the morphisms as R-modules. Currently our set of morphisms depends on the category, which if we keep the current setup, would give that Hom(V, W, Modules(RG)) is Hom(V, W, Modules(R)) (well, maybe only ==).

Actually...RG would be the category of group algebras whereas R would be in the category of rings, unless R was also the group algebra of some other group (okay, it's very contrived, but possible). My point is that just passing the category into Modules doesn't always tell us the full structure (of the homset), and is there some way we can work around this?

Well, as you say, we have two distinct categories:

    sage: Modules(Rings())

    sage: Modules(GroupAlgebras(Rings()))

so we are fine, aren't we?

Cheers,

Nicolas

comment:21 follow-up: Changed 8 months ago by tscrim

Until I make a group ring over a group ring. Actually, maybe a less contrived example of modules over S = R[a,b] / J where R = ZZ[x,y] / I for some ideals I, J. There should be more morphisms as R modules than as S modules. I think this could be a general issue anytime we try to do extension of scalars (or I'm being completely paranoid).

comment:22 Changed 8 months ago by git

  • Commit changed from 12afbe46ebafe56cf74661bb6d5ea81af583ba83 to 0397865de0a1b9636b7501f0d666f08473b148fb

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

8eaaa99Merge branch 'develop' into public/ticket/10963-doc-distributive
feab04amanual merge with 6.2.beta8
ce2193eMerge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
268345fTrac 10963: attempt at improving the wording for quotients/subquotients
729299bMerge sage 6.2.beta8 together with the latest version of #10963 in #15801
0397865Trac 15801: added warning about not yet fully implemented subcategory tests

comment:23 in reply to: ↑ 21 ; follow-up: Changed 8 months ago by nthiery

Replying to tscrim:

Until I make a group ring over a group ring.

What about:

    Modules(Groups().Algebras(Groups().Algebras()))

Actually, maybe a less contrived example of modules over S = R[a,b] / J where R = ZZ[x,y] / I for some ideals I, J. There should be more morphisms as R modules than as S modules. I think this could be a general issue anytime we try to do extension of scalars (or I'm being completely paranoid).

One thing is that each module M in Sage has a distinguished "base ring".
<digression>
In particular, M can't be in Modules(R) and in Modules(S)
simultaneously; this can be seen as a limitation of the current
category framework; no other CAS found a good solution to that
though
</digression>

So, if I have two modules M and N, Hom(M,N) actually depends on this
distinguished base ring (they should have the same!).

If I build M and N as R-modules, and M' and N' as S-modules, the fact
that they all belong to the same category only means that the code to
handle and compute the homsets will be the same, not that the homsets
Hom(M,N) and Hom(M',N') themselves will be the same.

Cheers,

Nicolas

comment:24 Changed 8 months ago by nthiery

  • Status changed from new to needs_review

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

Replying to nthiery:

Okay, I'm happy as long as we're getting things like this:

   Modules(Groups().Algebras(Groups().Algebras()))

for things like (QQ[G])[H] where G,H are groups.

One thing is that each module M in Sage has a distinguished "base ring".
<digression>
In particular, M can't be in Modules(R) and in Modules(S)
simultaneously; this can be seen as a limitation of the current
category framework; no other CAS found a good solution to that
though
</digression>

<digressing as well>This might improve as (if) we implement coercions between categories.</digressing as well>

So, if I have two modules M and N, Hom(M,N) actually depends on this
distinguished base ring (they should have the same!).

If I build M and N as R-modules, and M' and N' as S-modules, the fact
that they all belong to the same category only means that the code to
handle and compute the homsets will be the same, not that the homsets
Hom(M,N) and Hom(M',N') themselves will be the same.

Ah, I see. You must construct new modules M' and N' in order to consider morphisms as S-modules. I'm good with everything (conceptually).

Simon, Nils, (or anyone else,) did either of you verify Nicolas' above (or have any objections)?

comment:26 Changed 8 months ago by git

  • Commit changed from 0397865de0a1b9636b7501f0d666f08473b148fb to d396719ca5442e41259b16f43994e87099fdfcea

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

18a4a74Trac 10963: improve explanation of Sage notion of subcategories
5c271c9Trac 10963: improve explanation of Sage notion of subcategories (proofreading/rewording)
d0664d1Trac 10963: three typo fixes suggested by Darij
3009d90Trac 10963: little documentation improvements suggested by Darij
02db670Trac 10963: minor edits to "On the category hierarchy"
3bb48cbMerge branch 'develop = 6.2.rc0' into categories/axioms-10963
d396719Merge branch '6.2.rc0' and categories/axioms-10963 into categories/over_a_base_category-15801

comment:27 in reply to: ↑ 25 Changed 8 months ago by nthiery

Replying to tscrim:

Replying to nthiery:
Okay, I'm happy as long as we're getting things like this:

   Modules(Groups().Algebras(Groups().Algebras()))

for things like (QQ[G])[H] where G,H are groups.

For now the group algebra construction still uses categories over base
rings. But yes, that's what would happen otherwise.

Thanks for the feedback!

comment:28 Changed 8 months ago by git

  • Commit changed from d396719ca5442e41259b16f43994e87099fdfcea to 6a960b2d827dc5dbd0afb81fa6c1f2e402e25db8

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

a432952Trac 10963: trivial doctest update w.r.t. #15240 (Switch lattice polytopes to point collections)
dc87588clarifying Subobjects and Quotients interfaces
be5150fsome questions
3a0e8f7clarification of my clarification
86235d4Trac 10963: clarification^3 about subobjects
08ea069Trac 10963: suppressed now useless category example MyGroupAlgebra + tiny doc improvement
1441304some final fixes
ab4562cMerge branch 'public/ticket/10963-doc-distributive' into public/categories/over-a-base-ring-category-15801
5b8438eFixing up doc and some other minor things.
6a960b2Fixed Sets.Algebras().

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

I've merged in the latest #10963, made some misc doctweaks (added some doctests to/near functions were editing), fixed an issue with Sets.Algebras not taking a category (so GroupAlgebras(Rings()) used to fail). If you're happy with my changes, then I think this part is a positive review.

comment:30 Changed 8 months ago by git

  • Commit changed from 6a960b2d827dc5dbd0afb81fa6c1f2e402e25db8 to 1f09b8bde256c00bab2359e799b88e9da2273720

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

16ec0f3some more question(able edits)
2322225Trac 10963: Answer's to Darij's questions + minor edits + suppressed broken Monoids.Algebras.ParentMethods.an_element
c76743fMerge branch 'categories/axioms-10963' into categories/over_a_base_category-15801
1f09b8bTrac 15801: proofread Travis's comments + minor doctest improvements

comment:31 in reply to: ↑ 29 Changed 8 months ago by nthiery

Replying to tscrim:

I've merged in the latest #10963, made some misc doctweaks (added some doctests to/near functions were editing), fixed an issue with Sets.Algebras not taking a category (so GroupAlgebras(Rings()) used to fail). If you're happy with my changes, then I think this part is a positive review.

Cool, thanks! Please proofread my proofreading :-)

I have merged the latest #10963 since I was not sure it would not conflict with my changes.

Running all doctests now with the little tweak from #15919.

Cheers,

Nicolas

comment:32 Changed 8 months ago by tscrim

If all doctests pass for you, then positive review for this.

comment:33 Changed 8 months ago by nthiery

  • Status changed from needs_review to needs_work
  • Work issues set to Fix failing test in modules_with_basis.py

Shoot, there is one failing doctest which I somehow missed. Some Homset is missing a base_ring method which causes it not to be detected properly as being in its category. I'll try to fix this tomorrow or so.

Cheers,

Nicolas

comment:34 Changed 8 months ago by git

  • Commit changed from 1f09b8bde256c00bab2359e799b88e9da2273720 to 2114d54f98b80c9542b61051600d9ff686f99f13

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

55a36d6post-apocalyptic edits (correct this time)
72663b7Merge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
67cbc90Merge branch 'public/ticket/10963-doc-distributive' into public/categories/over-a-base-ring-category-15801
655b1f8Trac 10963: Proofread latest changes by Darij and Travis + minor doc edits
14d4449Trac 10963: Minor doc edit
9d7762fTrac 10963: fixed typo
70e7b32Fixed some more typos and back to "if not blah:".
655fc1eMerge branch 'public/ticket/10963-doc-distributive' into public/categories/over-a-base-ring-category-15801
ac91ed0Added _Hom_() method for Modules.
2114d54Merge branch 'public/categories/over-a-base-ring-category-15801' of trac.sagemath.org:sage into public/categories/over-a-base-ring-category-15801

comment:35 Changed 8 months ago by tscrim

  • Authors set to Nicolas M. Thiéry
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_work to needs_review
  • Work issues Fix failing test in modules_with_basis.py deleted

I've fixed it by implementing a _Hom_() for Modules (see commit ac91ed0). This might be more of a hack solution, but it works.

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

comment:36 Changed 8 months ago by nthiery

Ah, yes, that could one way to handle it. Thanks!

Sorry for the silence. I have been toying with a variant of it,
putting a similar test in the Homset constructor. I don't know which
one is best. Yours is more local context-wise (it only affects
modules); mine is more local code wise.

However while doing this I stumbled on another issue, which is that
now X in category is not necessarily equivalent to
X.category().is_subcategory(category). This is ok theoretically,
but there is at least one spot in the code, namely in Hom, where this
has to be fixed, which in turn opens a can of pickling worms.
Hopefully I'll have a proposal later tonight.

Cheers,

Nicolas

comment:37 Changed 8 months ago by nthiery

  • Dependencies changed from #10963 to #10963, #16275

I pushed my proposal to #16275.

Thanks to the use of X in category for sanity checks in Hom, we
can still handle things like:

    Hom(ZZ['x'], ZZ['x'], Modules(ZZ))

even if ZZx? is now in the category of Modules(Rings())
(which is not a subcategory of Modules(ZZ)).

I'll work tomorrow on double checking everything once #16275 is merged
in this ticket. I might end up preferring my hack rather than the
_Hom_ hack, because the whole _Hom_ protocol is not quite
satisfactory, and I'd rather avoid it when we can.

What do you think?

comment:38 follow-up: Changed 8 months ago by tscrim

I think we should leave my _Hom_ hack in because the homset is a module (when the codomain is a module as well) and at least we're somewhat demonstrating that when specifying that the homset has a base ring (through the Parent.__init__). So I'd say my _Hom_ and #16275 should coexist in the final version.

comment:39 in reply to: ↑ 38 Changed 8 months ago by nthiery

Replying to tscrim:

I think we should leave my _Hom_ hack in because the homset is a module (when the codomain is a module as well) and at least we're somewhat demonstrating that when specifying that the homset has a base ring (through the Parent.__init__). So I'd say my _Hom_ and #16275 should coexist in the final version.

Sorry, I was unclear. Yes, we definitely need some Hom hack to setup base_ring properly by passing it to init. My question is just whether this hack belongs to a custom _Hom_ method as you did or to Homset.__init__ as in my (yet unpushed) branch.

comment:40 Changed 8 months ago by git

  • Commit changed from 2114d54f98b80c9542b61051600d9ff686f99f13 to aa0159142f58b1feb9f2462aec4cc24a806165b3

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

1ff993b#16275: trivial commit to use format
0074f02#16275: added doctest illustrating an unpickling where Hom is called on an uninitialized parent
c1e91f3#16275: update ticket number in the comments
ae3ca80This is handled as well by the generic of Homset.__reduce__ method,
507ee00Trac 16275: fixes for the previous commit; btw its title should have read: removed sage.modular.abvar.homspace.Homspace.__reduce__
6f55aa2Merge branch 't/16275/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic' into categories/over_a_base_category-15801
fe1c1c5Trac 10963: Removed the old _join helper function in category.py (why was it still there???)
d6dc1fdMerge branch 'categories/axioms-10963' into categories/over_a_base_category-15801
54c3d67#15801: Initialize the base ring for module homsets
aa01591#15801: doctests for CategoryObjects.base_ring + docfix in Modules.SubcategoryMethods.base_ring

comment:41 Changed 8 months ago by nthiery

I just pushed my branch. Hmm, that was a bit bruteforce, sorry: I should probably have pushed it under a different name since it does not include your change ac91ed0.

See 54c3d67 for the alternative I propose; does it sound ok?


Last 10 new commits:

1ff993b#16275: trivial commit to use format
0074f02#16275: added doctest illustrating an unpickling where Hom is called on an uninitialized parent
c1e91f3#16275: update ticket number in the comments
ae3ca80This is handled as well by the generic of Homset.__reduce__ method,
507ee00Trac 16275: fixes for the previous commit; btw its title should have read: removed sage.modular.abvar.homspace.Homspace.__reduce__
6f55aa2Merge branch 't/16275/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic' into categories/over_a_base_category-15801
fe1c1c5Trac 10963: Removed the old _join helper function in category.py (why was it still there???)
d6dc1fdMerge branch 'categories/axioms-10963' into categories/over_a_base_category-15801
54c3d67#15801: Initialize the base ring for module homsets
aa01591#15801: doctests for CategoryObjects.base_ring + docfix in Modules.SubcategoryMethods.base_ring

comment:42 Changed 8 months ago by tscrim

  • Status changed from needs_review to positive_review

Your proposal will work since any category with has a WithBasis should have a forgetful functor to modules.

comment:43 Changed 8 months ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:44 Changed 7 months ago by git

  • Commit changed from aa0159142f58b1feb9f2462aec4cc24a806165b3 to 79f476698d8658ca8649ab765de00995c24f5455
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:

7e07dedTrac 10963: Fixed crosslinks
250c8faMerge branch 'public/ticket/10963-doc-distributive' of git://trac.sagemath.org/sage into 10963
16b8e0fdocumentation edits to clarify that super_categories should not contain anything twice
bbfcbb8Merge Darij's commit in 'public/ticket/10963-doc-distributive' into categories/axioms-10963
f22508bTrac 10963: proofread Darij's edits
8da7522Trac 10963: degraded a bit a newly added cross-link to work around 'make doc-clean; make clean' failure
4ef8b27Merge branch 'public/ticket/10963-doc-distributive' of git://trac.sagemath.org/sage into 10963new
afb911ccorrected conflict resolution
82c8ee0Merge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into public/ticket/10963-doc-distributive
79f4766Merge branch 'public/categories/over-a-base-ring-category-15801' of trac.sagemath.org:sage into public/categories/over-a-base-ring-category-15801

comment:45 Changed 7 months ago by tscrim

Rebased over rebased #10963.

comment:46 Changed 7 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:47 Changed 7 months ago by vbraun

  • Status changed from positive_review to needs_work

I get lots of doctest failuers, e..g NotImplementedError: Pieri factors for type ['A', 3, 1]. Might be this ticket or #10963. On top of 6.3.beta0.

comment:48 Changed 7 months ago by nthiery

Is this with #14102 applied? Could this be due to the import statement that was accidently removed and then reinserted in ​1579b88. See http://trac.sagemath.org/ticket/14102#comment:72.

Cheers,

Nicolas

comment:49 Changed 7 months ago by vbraun

No, #14102 wasn't applied since it depends on this ticket.

comment:50 Changed 7 months ago by git

  • Commit changed from 79f476698d8658ca8649ab765de00995c24f5455 to 281f5392e81e98d835ddf249325e66c1c36a08cc

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

281f539Added back in import statement as per comment.

comment:51 Changed 7 months ago by nthiery

Ah, I see: the accidental removal of import occured in this branch (commit 41768a3), and was reverted in the followup branch #14102 (commit ​1579b88). I cherry-picked back 1579b88 to this branch. This should work now. Running the tests.

Thanks for your report!

Cheers,

Nicolas

comment:52 Changed 7 months ago by nthiery

  • Status changed from needs_work to positive_review

comment:53 Changed 7 months ago by vbraun

  • Status changed from positive_review to needs_work
  • Work issues set to doc-pdf

make doc-pdf fails (probably due to #10963)

comment:54 follow-up: Changed 7 months ago by vbraun

Also

sage -t --long src/sage/quivers/path_semigroup.py  # 1 doctest failed
sage -t --long src/sage/quivers/representation.py  # 6 doctests failed

comment:55 in reply to: ↑ 54 Changed 7 months ago by nthiery

Replying to vbraun:

Also

sage -t --long src/sage/quivers/path_semigroup.py  # 1 doctest failed
sage -t --long src/sage/quivers/representation.py  # 6 doctests failed

Indeed, I just found out about this integration failure with #12630, and posted a question there:

http://trac.sagemath.org/ticket/12630#comment:270

comment:56 Changed 7 months ago by git

  • Commit changed from 281f5392e81e98d835ddf249325e66c1c36a08cc to dcb11d850530d23e1d8867daa84ade0e04d7ce15

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

96c631fMerge branch 'develop' into categories/axioms-10963
b1a2aedTrac 10963: two typo fixes to let the pdf documentation compile
c16f18bMerge branch 'public/ticket/10963-doc-distributive' of trac.sagemath.org:sage into categories/axioms-10963
dcb11d8Merge branch 'categories/axioms-10963' into categories/over_a_base_category-15801

comment:57 Changed 7 months ago by nthiery

Pdf compilation fixed in #10963 and merged here.

comment:58 follow-ups: Changed 7 months ago by SimonKing

  • Cc darij added

I think the quiver issue should be discussed here.

Nicolas, do you agree with Darij that quiver representations over QQ should live in Modules(QQ), not in Modules(QuiverAlgebra)? If this is so, then hopefully it will be easy to switch.

comment:59 in reply to: ↑ 58 Changed 7 months ago by nthiery

Replying to SimonKing:

I think the quiver issue should be discussed here.

Ok.

Nicolas, do you agree with Darij that quiver representations over QQ should live in Modules(QQ), not in Modules(QuiverAlgebra)? If this is so, then hopefully it will be easy to switch.

No strong opinion either way, but yes, I would lean in this direction
since we don't have yet a proper category hierarchy for
representations. So in the mean time, it makes sense to reserve
Modules(K) for what we have used it so far: doing linear algebra
over K, ...

If it makes it easier to switch, so much the better :-)

Cheers,

Nicolas

comment:60 Changed 7 months ago by git

  • Commit changed from dcb11d850530d23e1d8867daa84ade0e04d7ce15 to 15658bd13f22b874423f62b8e6e6fb4d4f7b56ec

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

15658bdTrac 15801: fixed merge with #12630

comment:61 in reply to: ↑ 58 Changed 7 months ago by nthiery

Replying to SimonKing:

Nicolas, do you agree with Darij that quiver representations over QQ should live in Modules(QQ), not in Modules(QuiverAlgebra)? If this is so, then hopefully it will be easy to switch.

Done. Please review! Running all long tests now.

comment:62 Changed 7 months ago by SimonKing

Sorry for my being too slow. I wanted to try and fix it, but was busy with some other tickets. Thank you for doing it, and I'll now try to review.

comment:63 Changed 7 months ago by SimonKing

I read the latest commit, and it seems good. Hence, I wait for the test results...

comment:64 Changed 7 months ago by SimonKing

The pdf does build...

comment:65 Changed 7 months ago by SimonKing

  • Work issues doc-pdf deleted

I get numerous errors of the following type:

File "src/sage/dev/sagedev.py", line 4836, in sage.dev.sagedev.SageDev._new_local_branch_for_ticket
Failed example:
    dev, config, UI, server = single_user_setup()
Expected nothing
Got:
    GitError: git exited with a non-zero exit code (1).
    This happened while executing "git -c user.email=doc@test.test -c user.name=doctest pull /home/king/.sage/temp/linux-etl7.site/1419/dir_y4yz1q/repo master".
    git printed nothing to STDOUT.
    git printed the following to STDERR:
    git: 'pull' is not a git command. See 'git --help'.
    <BLANKLINE>
    Did you mean this?
        shell

Never seen such errors before.

Last edited 7 months ago by SimonKing (previous) (diff)

comment:66 Changed 7 months ago by SimonKing

Is this perhaps an issue of sage-6.3.beta0?

comment:67 follow-up: Changed 7 months ago by nthiery

Thanks Simon for reviewing this! Could you have a quick look as well at the small last commit on #10963?

For the git error, IIRC you need a recent version of git for all tests to pass, and doing
sage -i git should do the job.

Cheers,

Nicolas

comment:68 follow-up: Changed 7 months ago by darij

Simon, are you using sage git? There is always plain git, and I fear that it'll stay the more reliable option for a long time...

comment:69 follow-up: Changed 7 months ago by tscrim

  • Status changed from needs_work to positive_review

Tests pass for me, so I'm going to set this to positive review.

I also only use my system-wide git.

comment:70 in reply to: ↑ 69 Changed 7 months ago by SimonKing

Replying to tscrim:

I also only use my system-wide git.

Are you sure that you use your system-wide git when you run the tests in sage.dev? Anyway, my system-wide git does know about git pull---only Sage's git doesn't know.

Best regards,
Simon

comment:71 in reply to: ↑ 68 Changed 7 months ago by SimonKing

Replying to darij:

Simon, are you using sage git? There is always plain git, and I fear that it'll stay the more reliable option for a long time...

I am now talking about the git that I use, but about the git that sage -dev uses.

comment:72 in reply to: ↑ 67 Changed 7 months ago by SimonKing

Replying to nthiery:

For the git error, IIRC you need a recent version of git for all tests to pass, and doing
sage -i git should do the job.

It doesn't, because git is already installed. sage -f git doesn't work either. The resulting git still doesn't know about the pull command.

comment:73 Changed 7 months ago by vbraun

  • Branch changed from public/categories/over-a-base-ring-category-15801 to 15658bd13f22b874423f62b8e6e6fb4d4f7b56ec
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.