sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.05k stars 385 forks source link

Axioms and more functorial constructions #10963

Closed nthiery closed 9 years ago

nthiery commented 13 years ago

This ticket implements:

    sage: Groups() & Sets().Finite()
    Category of finite groups

    sage: Algebras(QQ).Finite() & Monoids().Commutative()
    Category of finite commutative algebras over Rational Field

    sage: (Monoids() & CommutativeAdditiveGroups()).Distributive()
    Category of rings

    sage: Rings().Division() & Sets().Finite()
    Category of finite fields

This ticket is dedicated to the town of Megantic where I was so warmly welcomed and a good chunk of this ticket got implemented!

Depends on #11224 Depends on #8327 Depends on #10193 Depends on #12895 Depends on #14516 Depends on #14722 Depends on #13589 Depends on #14471 Depends on #15069 Depends on #15094 Depends on #11688 Depends on #13394 Depends on #15150 Depends on #15506 Depends on #15757 Depends on #15759 Depends on #16244 Depends on #16269

CC: @sagetrac-sage-combinat @simon-king-jena @saliola @anneschilling @vbraun @nbruin @zabrocki

Component: categories

Keywords: days54

Work Issues: To be merged simultaneously with #15801

Author: Nicolas M. Thiéry

Branch: c16f18b

Reviewer: Volker Braun, Nils Bruin, Peter Bruin, Frédéric Chapoton, Darij Grinberg, Florent Hivert, Simon King, Travis Scrimshaw

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

nthiery commented 13 years ago

Dependencies: #11224

nthiery commented 13 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,20 @@
-The patch under development on the Sage-Combinat queue implements:
+The patch under finalization on the Sage-Combinat queue implements:

-- The algebra of a finite enumerated set is a finite dimensional algebra
-- Commutative additive semigroup and monoid algebras
+- Support for full subcategories defined by a predicate on the objects
+  (Finite, Infinite, FiniteDimensional, Commutative, Graded, Facade),
+  and joins thereof:
+
+```
+    sage: Category.join([Groups(), Sets().Finite()])
+    Category of finite groups
+    sage: Category.join([Algebras(QQ).Finite(), Monoids().Commutative()])
+    Join of Category of commutative algebras over Rational Field and Category of finite monoids
+```
+
+- More mathematical rules:
+  - A subquotient of a finite set is a finite set
+  - The algebra of a finite set is finite dimensional
+  - The algebra of a commutative magma is commutative
+- Algebras of commutative additive semigroups and monoids
 - More documentation for IsomorphicObjects and other doc improvements
stumpc5 commented 12 years ago
comment:3

Hi Nicolas,

How far is this patch -- I just saw that the UCF patch depends on this.

I didn't actually figure out how it depends, I just get a trivial rebase, and then an import loop which wasn't easily fixable. The problem was my use of CombinatorialFreeModule...

Thx, Christian

stumpc5 commented 12 years ago

Changed dependencies from #11224 to #11224, #8327

simon-king-jena commented 12 years ago
comment:6

It turns out that many hunks of that patch fail to apply, when one starts with sage-4.8.alpha0 plus the new cython spkg from #11761 plus the rebased patch from #8327. So, there are further dependencies from the combinat queue. Could you try to find out which they are, and post them here?

simon-king-jena commented 12 years ago

Work Issues: Find dependencies

simon-king-jena commented 12 years ago

Changed work issues from Find dependencies to Find dependencies. Finite dimensional vector spaces

simon-king-jena commented 12 years ago
comment:7

Working with the combinat queue, I could do some first tests. What I find very strange is the fact that the category of vector spaces coincides with the category of finite-dimensional vector spaces:

sage: VectorSpaces(QQ).FiniteDimensional() is VectorSpaces(QQ)
True

Is that really intended? I thought that the idea of this ticket is to create new categories dynamically. Hence, even though there previously was no specific implementation of the category of finite dimensional vector spaces, the construction VectorSpaces(QQ).FiniteDimensional() would automatically create one. Or am I misunderstanding something?

dimpase commented 11 years ago
comment:10

Would you mind actually uploading the patch in question here?

nthiery commented 11 years ago

Description changed:

--- 
+++ 
@@ -18,3 +18,4 @@
 - Algebras of commutative additive semigroups and monoids
 - More documentation for IsomorphicObjects and other doc improvements

+See http://combinat.sagemath.org/patches/file/tip/trac_10963-more_functorial_constructions-nt.patch and follow ups.
nthiery commented 10 years ago
comment:12

Little update: after two good weeks of work, here is the status of the patch in the Sage-Combinat queue:

In short: getting ready for review next week!

nthiery commented 10 years ago

Changed dependencies from #11224, #8327 to #11224, #8327, #10193 #12895, #14516, #14722

nthiery commented 10 years ago

Changed dependencies from #11224, #8327, #10193 #12895, #14516, #14722 to #11224, #8327, #10193, #12895, #14516, #14722, #13589

nthiery commented 10 years ago

Changed work issues from Find dependencies. Finite dimensional vector spaces to none

simon-king-jena commented 10 years ago
comment:15

Just for the record: I currently have applied

trac_12876_category_abstract_classes_for_hom.patch
trac11935_weak_pickling_by_construction-nt.patch
trac_11935-weak_pickling_by_construction-review-ts.patch
trac_12895-subcategory-methods-nt.patch
trac_12895-review.patch
trac_10193-graded_sets-rebased.patch
trac_10193-graded_sets-review-ts.patch
trac_13589-categories-c3_under_control-nt.patch
trac13589_cmp_key_attribute.patch
trac13589_improve_startuptime.patch
trac_12630_quivers_v2.patch
trac12630_refactor_code.2.patch
trac_14722-lazy_import_at_startup-nt.patch
trac_14266_ascii_art_13_05_15_EliX-jbp.patch
trac_14266-ascii_art-review-ts.patch
trac_14266_terminal_width.patch
trac_14402-tensor_product_infinite_crystals-ts.patch
trac_14143-alcove-path-al.3.patch
trac_14413-elementary_crystals-bs.patch
trac_14516-crystals_speedup-ts.patch

on top of sage-5.10.rc1 (I think these are all dependencies).

So, as soon as Nicolas told me how to get the patch from git and what is meant by "and follow-ups", I can start reviewing!

simon-king-jena commented 10 years ago
comment:16

With these list I gave above, the patch does not apply. Some of it might be to blame the latest version of my trac13589_improve_startuptime.patch. So, let's try to remove this. But there seem to be further dependencies.

simon-king-jena commented 10 years ago
comment:17

Even when I remove trac13589_improve_startuptime.patch, I still get 4 mismatches and 1 noise in category.py, 4 mismatches in category_singleton.pyx, and 1 mismatch and 2 noises in c3_controlled.pyx

simon-king-jena commented 10 years ago
comment:18

Back at work. These patches on top of sage-5.11.b3 do apply:

trac_14516-crystals_speedup-ts.2.patch
trac_14722-lazy_import_at_startup-nt.patch
trac_13589-categories-c3_under_control-nt.patch
trac_10963-more_functorial_constructions-nt.patch

(the last patch applies with a little fuzz)

However, if we decide to include the two additional patches from #13589, then the last patch needs to be rebased.

For now, I'll try without the two additional patches, since they only concern performance (and seem to have disappointingly little effect).

simon-king-jena commented 10 years ago

Reviewer: Simon King

nthiery commented 10 years ago
comment:19

Hi Simon!

Great that the patches apply.

I am happy to handle the rebase on top of the extra patches for #13589. I also have some modifications in primer.py that I need to finish merging it. I'll try to finish this today. I guess there is enough to review elsewhere to keep you busy until then :-)

Thanks a lot!

Cheers, Nicolas

simon-king-jena commented 10 years ago

Work Issues: Rebase wrt. #13589

simon-king-jena commented 10 years ago
comment:21

Just to make sure I understand correctly: During __init__ of a group algebra, only the coercion from the group is registered, since the coercion from the base ring is registered during __init_extra__, which is obtained from the category?

nthiery commented 10 years ago
comment:22

Replying to @simon-king-jena:

Just to make sure I understand correctly: During __init__ of a group algebra, only the coercion from the group is registered, since the coercion from the base ring is registered during __init_extra__, which is obtained from the category?

Yes indeed!

There is nothing specific to do for GroupAlgebras about this feature, since it's already provided by Algebras.

simon-king-jena commented 10 years ago
comment:23

There is now a category of non-associative algebras. But that's misleading, because it certainly contains all associative algebras too, isn't it? I'd say that "non-associative non-unital (non-commutative) (non-finite-dimensional) algebras" should simply be "algebras".

In other words, I am against mentioning the absence of an axiom in the category name. Only the presence of an axiom must play a role.

nthiery commented 10 years ago
comment:24

Replying to @simon-king-jena:

There is now a category of non-associative algebras. But that's misleading, because it certainly contains all associative algebras too, isn't it? I'd say that "non-associative non-unital (non-commutative) (non-finite-dimensional) algebras" should simply be "algebras".

In other words, I am against mentioning the absence of an axiom in the category name. Only the presence of an axiom must play a role.

Yeah, that's been a recurrent issue. I agree that this is not nice, even though it's relatively common practice in maths to label as "non-foo things" the larger field of study where one is interested in things that are "not necessarily foo". For non associative non unital algebras, Florent mentionned yesterday that "magmatic algebras" was fairly standard, and I am happy to go with it. Do you have a better name for "non unital algebras"? I am not really keen on "NotNecessarilyUnitalAlgebras".

Cheers,

simon-king-jena commented 10 years ago
comment:25

Replying to @nthiery:

Do you have a better name for "non unital algebras"? I am not really keen on "NotNecessarilyUnitalAlgebras".

Yes. A not necessarily unital not necessarily associative not necessarily finite-dimensional not necessarily noetherian not necessarily ... is commonly known as an algebra.

In other words, I suggest to name the categories exactly parallel to the axioms it provides. Actually, before reading your patch, I thought that you aim to automatically create a category of "associative algebras", given the category of algebras and the axiom "associative".

Hence, I think it should be

                  algebras
                /        \
 associative algebras   unital algebras
               \          /
        associative unital algebras

and similar for commutative algebras, commutative associative algebras, commutative associative unital algebras, and so on.

nthiery commented 10 years ago
comment:26

Replying to @simon-king-jena:

Yes. A not necessarily unital not necessarily associative not necessarily finite-dimensional not necessarily noetherian not necessarily ... is commonly known as an algebra.

Yup, and that's indeed what Wikipedia says which is a good point. However in many textbooks and other pieces of literature "algebra" implicitly includes "associative" and "unital" (for the same reason that it will be heavy for us to write almost everywhere Algebras().Associative().Unital()).

More importantly: changing the semantic current "Algebras" in Sage would be seriously backward incompatible. And we would have to think about what we want to do about categories like "HopfAlgebras" to keep things consistent.

So I definitely see your point but at this point I am not keen on opening yet another can of worms (both technical and social) to this already too big patch.

Actually, before reading your patch, I thought that you aim to automatically create a category of "associative algebras", given the category of algebras and the axiom "associative".

Up to the names, that's precisely what's its doing :-)

Hence, I think it should be

                  algebras
                /        \
 associative algebras   unital algebras
               \          /
        associative unital algebras

What about, at least as a temporary measure, going for:

                      magmatic algebras
                /                           \
 associative magmatic algebras   unital magmatic algebras
                \                           /
                           algebras

(or any other not-yet-used name you like instead of "magmatic algebra")

Cheers, Nicolas

simon-king-jena commented 10 years ago
comment:27

Replying to @nthiery:

However in many textbooks and other pieces of literature "algebra" implicitly includes "associative" and "unital"

Certainly there also exist textbooks that will for simplicity say "algebra" when they in fact mean "commutative algebra". But I would expect that all these textbooks state at some point the definition of (plain) algebras and later say that "for simplicity" or "unless stated otherwise" they assume whatever additional axioms.

And even "better": There were times when a certain algebraic community would only talk about finite groups. I recently heard colleagues talk about these times. It was like "they provided generators and relations and then needed to prove that it is a group", which in today's language is "they provided a group presentation and needed to prove that the group is finite".

You see: There are certain conventions peculiar to certain fields of research.

But I think a general computer algebra system should not be biased towards any of these peculiar conventions. Hence, it should use the "greatest common divisor" of the notions, which is: An R-algebra is a an R-module and a multiplicative magma, such that multiplication is R-bilinear.

(for the same reason that it will be heavy for us to write almost everywhere Algebras().Associative().Unital()).

We can certainly have a short-cut for defining this thing.

More importantly: changing the semantic current "Algebras" in Sage would be seriously backward incompatible.

Backward compatibility is indeed important. It would be difficult to switch from Algebras in the current Sage-use to Algebras in the (I think) normal mathematical use.

However, I do think that to the very very least we should let Algebras() print as "Category of unital associative algebras".

And we would have to think about what we want to do about categories like "HopfAlgebras" to keep things consistent.

Wikipedia does not assume associativity for algebras, but it does assume co-associativity for co-algebras. Weird.

So I definitely see your point but at this point I am not keen on opening yet another can of worms (both technical and social) to this already too big patch.

Concerning social: I vividly remember many talks in the séminaire quantique in Strasbourg, entitled along the lines of "quasi-commutative quasi-cocommutative quasi-Hopf algebras". I think these guys would be unhappy about tacitly assuming too many axioms for algebras. And I just checked: There also is the notion of quasi-associative algebras in literature...

What about, at least as a temporary measure, going for:

                      magmatic algebras
                /                           \
 associative magmatic algebras   unital magmatic algebras
                \                           /
                           algebras

(or any other not-yet-used name you like instead of "magmatic algebra")

I have never heard about "magmatic algebras" before. But I have no better idea ("plain algebras"?).

Sage-devel poll? Sage-algebra poll (although this list seems dead)? Sage-combinat-devel poll?

nthiery commented 10 years ago
comment:28

Replying to @simon-king-jena:

Sage-devel poll? Sage-algebra poll (although this list seems dead)? Sage-combinat-devel poll?

I think a CC to all three is in order in this case. I'll try to launch the poll tomorrow.

Thanks for your work on the review!

nthiery commented 10 years ago

Changed work issues from Rebase wrt. #13589 to Finish merging some changes in the primer

nthiery commented 10 years ago
comment:29

Patch rebased on top of #13589

simon-king-jena commented 10 years ago
comment:30

The patch applies with fuzz, but it does apply.

simon-king-jena commented 10 years ago
comment:31

Why is there a double-underscore __neg__ method as element method of additive groups? The reason for the single underscore arithmetic methods is, to my understanding, to enable the coercion model. But the coercion model is not involved in the case of __neg__, isn't it? Hence, I think one should keep it double underscore, and should not ask for an implementation via a single underscore method.

simon-king-jena commented 10 years ago
comment:32

Mental note: A lot of things happen when joining categories. I recall that in some examples forming the join of categories was the reason for slowness in algebraic constructions. Hence, we should have a look at the speed.

simon-king-jena commented 10 years ago
comment:33

I see that you define a method _cmp_key(self) for join categories, that just tells that one shouldn't call it on join categories. That's bad, because meanwhile _cmp_key is a lazy attribute (or an optimized version of a lazy attribute), hence, it is no method anyway. Can we remove this method?

simon-king-jena commented 10 years ago
comment:34

You ask:

# TODO: find a better way to check that cls is an abstract class 

What about a class attribute? Something like

"abstract" in cls.__base__.__dict__

Or is it generally not the case that cls.__base__ coincides with cls.__mro__[1]?

simon-king-jena commented 10 years ago
comment:35

make ptest resulted in

sage -t devel/sage/sage/geometry/polyhedron/plot.py  # 1 doctest failed
sage -t devel/sage/sage/categories/category.py  # 3 doctests failed
sage -t devel/sage/sage/quivers/free_small_category.py  # 2 doctests failed
sage -t devel/sage/sage/categories/category_with_axiom.py  # 1 doctest failed
simon-king-jena commented 10 years ago
comment:36

I don't see why the patchbot had trouble applying the patch. Let's kick it:

Apply trac_10963-more_functorial_constructions-nt.patch

simon-king-jena commented 10 years ago
comment:37

Replying to @simon-king-jena:

make ptest resulted in

sage -t devel/sage/sage/geometry/polyhedron/plot.py  # 1 doctest failed
sage -t devel/sage/sage/categories/category.py  # 3 doctests failed
sage -t devel/sage/sage/quivers/free_small_category.py  # 2 doctests failed
sage -t devel/sage/sage/categories/category_with_axiom.py  # 1 doctest failed

Yes, as you can see, I have #12630 applied as well. But this only introduces new modules, but does not interfere with old modules. Hence, I don't think the errors come from this.

simon-king-jena commented 10 years ago
comment:38

Here are the failures in detail: The first is noise:

sage -t devel/sage/sage/geometry/polyhedron/plot.py
    [178 tests, 5.55 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 5.8 seconds
    cpu time: 4.3 seconds
    cumulative wall time: 5.6 seconds

The second:

sage -t devel/sage/sage/categories/category.py
**********************************************************************
File "devel/sage/sage/categories/category.py", line 1940, in sage.categories.category.Category.join
Failed example:
    type(TCF)
Expected:
    <class 'sage.categories.category_with_axiom.TestObjects.Commutative.Facade_with_category'>
Got:
    <class 'sage.categories.category_with_axiom.Commutative.Facade_with_category'>
**********************************************************************
File "devel/sage/sage/categories/category.py", line 1950, in sage.categories.category.Category.join
Failed example:
    type(TCF)
Expected:
    <class 'sage.categories.category_with_axiom.TestObjects.Commutative.FiniteDimensional_with_category'>
Got:
    <class 'sage.categories.category_with_axiom.Commutative.FiniteDimensional_with_category'>
**********************************************************************
File "devel/sage/sage/categories/category.py", line 1963, in sage.categories.category.Category.join
Failed example:
    type(TUCF)
Expected:
    <class 'sage.categories.category_with_axiom.TestObjects.FiniteDimensional.Unital.Commutative_with_category'>
Got:
    <class 'sage.categories.category_with_axiom.Unital.Commutative_with_category'>
**********************************************************************
1 item had failures:
   3 of  47 in sage.categories.category.Category.join
    [388 tests, 3 failures, 6.89 s]
----------------------------------------------------------------------
sage -t devel/sage/sage/categories/category.py  # 3 doctests failed
----------------------------------------------------------------------
Total time for all tests: 7.3 seconds
    cpu time: 6.8 seconds
    cumulative wall time: 6.9 seconds

The third needs to be taken care of only if #12630 finally gets a review.

The last one:

sage -t devel/sage/sage/categories/category_with_axiom.py
**********************************************************************
File "devel/sage/sage/categories/category_with_axiom.py", line 755, in sage.categories.category_with_axiom.CategoryWithAxiom.__reduce__
Failed example:
    C.__class__
Expected:
    <class 'sage.categories.distributive_magmas_and_additive_magmas.DistributiveMagmasAndAdditiveMagmas.AdditiveAssociative.AdditiveCommutative_with_category'>
Got:
    <class 'sage.categories.distributive_magmas_and_additive_magmas.AdditiveAssociative.AdditiveCommutative_with_category'>
**********************************************************************
1 item had failures:
   1 of   8 in sage.categories.category_with_axiom.CategoryWithAxiom.__reduce__
    [179 tests, 1 failure, 0.25 s]

So, nothing dramatic.

simon-king-jena commented 10 years ago

Changed work issues from Finish merging some changes in the primer to Reduce startup time by 5%. Avoid "recursion depth exceeded (ignored)". Trivial doctest fixes.

simon-king-jena commented 10 years ago
comment:39

Patchbot finds

sage -t --long /mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/rings/number_field/number_field.py  # 1 doctest failed
sage -t --long /mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/doc/ru/tutorial/tour_groups.rst  # 1 doctest failed
sage -t --long /mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/geometry/polyhedron/plot.py  # 1 doctest failed
sage -t --long /mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/categories/category.py  # 3 doctests failed
sage -t --long /mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/categories/category_with_axiom.py  # 1 doctest failed

This sounds serious:

sage -t --long /mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/rings/number_field/number_field.py
**********************************************************************
File "/mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/rings/number_field/number_field.py", line 309, in sage.rings.number_field.number_field.?
Failed example:
    RR.coerce_map_from(K)
Expected:
    Composite map:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Real Field with 53 bits of precision
      Defn:   Generic morphism:
              From: Number Field in a with defining polynomial x^3 - 2
              To:   Real Lazy Field
              Defn: a -> 1.259921049894873?
            then
              Conversion via _mpfr_ method map:
              From: Real Lazy Field
              To:   Real Field with 53 bits of precision
Got:
    Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <function remove at 0x2820668> ignored
    Composite map:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Real Field with 53 bits of precision
      Defn:   Generic morphism:
              From: Number Field in a with defining polynomial x^3 - 2
              To:   Real Lazy Field
              Defn: a -> 1.259921049894873?
            then
              Conversion via _mpfr_ method map:
              From: Real Lazy Field
              To:   Real Field with 53 bits of precision
File "/mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/doc/ru/tutorial/tour_groups.rst", line 14, in doc.ru.tutorial.tour_groups
Failed example:
    G = PermutationGroup(['(1,2,3)(4,5)', '(3,4)'])
Expected nothing
Got:
    Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <function remove at 0xfe16e0> ignored
File "/mnt/storage2TB/patchbot/Sage/sage-5.11.beta3/devel/sage/sage/geometry/polyhedron/plot.py", line 461, in sage.geometry.polyhedron.plot.Projection.__init__
Failed example:
    p = polytopes.icosahedron()
Expected nothing
Got:
    Exception RuntimeError: 'maximum recursion depth exceeded while getting the str of an object' in <function remove at 0x2e03e60> ignored
    Exception RuntimeError: 'maximum recursion depth exceeded while getting the str of an object' in <function remove at 0x2e03e60> ignored

And the result of the startup time plugin is also a bad news.

+Average increase of 0.057 secs or 8.1%.

+With 100% confidence, startup time increased by at least 5%
+With 100% confidence, startup time increased by at least 2.5%
+With 100% confidence, startup time increased by at least 1%
+With 100% confidence, startup time increased by at least 0.5%
+With 100% confidence, startup time increased by at least 0.25%
simon-king-jena commented 10 years ago
comment:40

There is a naked

assert False 

in sage.categories.category.Category.__init__, followed(!) by a deprecation warning. How can the warning ever appear after asserting a false statement?

simon-king-jena commented 10 years ago
comment:41

I have a question on how to implement a new category with support of axioms.

If I understand correctly, the method _with_axiom_categories tells which categories need to be added to self in order to get the category with the axiom added. Is one supposed to overload this method? Sorry, I did not read your patch completely yet. Is this question answered somewhere? If not, I think it must be added to a tutorial.

simon-king-jena commented 10 years ago
comment:42

Here is some cryptic phrase from category_with_axiom:

The later two are implemented using respectively 
:meth:`__classcall__` and :meth:`__classget__` which see. 

It ends in the middle of the phrase.

simon-king-jena commented 10 years ago
comment:43

I try to summarize how I understand how the patch works---so, please correct me if I misunderstood.

Generally, adding an axiom A to a category C means: Forming the join of C with C._with_axiom_categories(A), unless there is a class getattr(C,A), for example: Magmas().Associative is Semigroups. The categories returned by C._with_axiom_categories(A) would, for example, provide new parent and element methods (such as: prod() only makes sense in an associative magma, _test_one() only makes sense for unital magmas).

If I understand correctly, the same "axiom category" (here: Magmas.Associative) is available to all subcategories of Magmas(), because it is defined in Magmas.SubcategoryMethods. Or am I mistaken and it is not the same? Is another dynamic class involved here? This could also give rise to slowness.

Apart from hard-coded cases, the rôle of JoinCategory has generally increased, and this indicates it might make sense to spend some optimization there. A highly significant increase of at least 5% of startup time is rather much. I don't know if JoinCategory is the only problem here.

simon-king-jena commented 10 years ago
comment:44

If I understand correctly, the reason for creating a JoinCategory is to get the correct supercategories. But there are alternative ways to get the supercategories right. I could imagine to use a dynamic class instead. So, the aim of this post is to present an alternative approach that avoids joins.

If C is a category and one wants to have C.MyAxiom(), then I suggest to create a dynamic class cls out of C.__class__ (and perhaps also using the class C.MyAxiom?), and set a class attribute cls._used_axioms which is a (frozen) set formed by C.__class__._used_axioms and "MyAxiom".

Note: The order in which the axioms are given should not matter. Hence, the way of caching the dynamic class should be: By a class that has no axioms, and by C.__class__._used_axioms.

We would like to call cls with the same __init__ arguments that were used for creating C. So, how to get the init data? No problem, since C uses UniqueRepresentation!. For example:

sage: C = Bimodules(ZZ, QQ)
sage: C._reduction
(sage.categories.bimodules.Bimodules, (Integer Ring, Rational Field), {})

So, C.MyAxiom() would eventually do something like this

   cls = dynamic_class("MyAxiom"+C.__class__.__name__, (C.__class__, C.MyAxiom), C.__class__, <take care of caching>)
   return cls(*(C._reduction[1][0]), **(C._reduction[1][1]))

Note that by way of caching the dynamic class, I guess the above would automatically cover the corner case that C.__class__._used_axioms contains "MyAxiom". Namely, in this case, cls is C.__class__ by means of caching the dynamic class, and then cls(*..., **...) coincides with C, since it is a UniqueRepresentation.

By means of explicitly overloading the cache of the dynamic class, one could even ensure that DivisionRings.Finite() returns Fields.Finite(), I guess.

Let's denote C2=C.MyAxiom(). And then, the critical question is: How to determine the super categories of C2?

I guess for each axiom A in C2.__class__._used_axioms, we want to return C2._without_axiom(A), and we want to return D._with_axiom(A) for all D is in C2._without_axiom(A).super_categories(), of course removing duplicates.

So, there only remains to answer: What is C2._without_axiom(A)?

Again, we can use C2._reduction to get the input data, but how to get the class of D=C2._without_axiom(A)? Note that C2 might have several axioms, and we do not order the axioms.

However, we know what D.__class__._used_axioms is supposed to look like: It is C2.__class__._used_axiom.difference("MyAxiom").

Thus, we get something like this:

@cached_method
def _without_axiom(self, axiom):
    if axiom not in self.__class__._used_axioms:
        <raise some error>
    new_axioms = self.__class__._used_axioms.difference([axiom])
    for cls in self.__class__.__mro__:
        if getattr(cls, "_used_axioms", None) == new_axioms:
            break
    if cls is object:
        <raise some error>
    return cls(*(self._reduction[1][0]), **(self._reduction[1][1]))

Do you think this would make sense?

nthiery commented 10 years ago
comment:45

Replying to @simon-king-jena:

sage -t devel/sage/sage/categories/category.py
**********************************************************************
File "devel/sage/sage/categories/category.py", line 1940, in sage.categories.category.Category.join
Failed example:
    type(TCF)
Expected:
    <class 'sage.categories.category_with_axiom.TestObjects.Commutative.Facade_with_category'>
Got:
    <class 'sage.categories.category_with_axiom.Commutative.Facade_with_category'>

}}}

Ah yes, good point: I have #9107 in my queue. So we will have to either add it as a dependency if it gets ready soon, or update the doctests. As you point out, nothing dramatic.

nthiery commented 10 years ago
comment:46

Replying to @simon-king-jena:

Why is there a double-underscore __neg__ method as element method of additive groups? The reason for the single underscore arithmetic methods is, to my understanding, to enable the coercion model. But the coercion model is not involved in the case of __neg__, isn't it? Hence, I think one should keep it double underscore, and should not ask for an implementation via a single underscore method.

All I did is to lift this method from "sage.structure.element.ModuleElement", as a step toward deprecating this class.

I agree that the _neg_ feature itself is questionable (it has no purpose besides consistency). So one could think about removing it (and fixing the couple modules in Sage that implement _neg_). But that would require a discussion on sage-devel and is in any cases for a different ticket.

For this ticket, do you think I should add a little comment about this in the doc?

nthiery commented 10 years ago
comment:47

Hi Simon!

Thanks for all your work on the review of this ticket! I am currently on vacations, so my answers might be slow.

Replying to @simon-king-jena:

If I understand correctly, the reason for creating a JoinCategory is to get the correct supercategories.

The reason to call "join" is indeed to get the correct supercategories for C.MyAxiom(). Note that, on the other hand and unless I screwed up somewhere, there should be no JoinCategory produced (unless of course the end result of C.MyAxiom() itself is such a JoinCategory).

But there are alternative ways to get the supercategories right. I could imagine to use a dynamic class instead. So, the aim of this post is to present an alternative approach that avoids joins.

In general, I agree that joins are called quite often and it would be nice to optimize them and/or call them less often. However, I think we really want to call a join to get the full power of the architecture. Imagine for example that:

Then we want C.MyAxiom().super_categories() to automatically include B.MyOtherAxiom(), for otherwise we would need to basically replicate the information that A.MyAxiom() implies A.MyOtherAxiom() over and over in subcategories, and this would not scale.

Handling this kind of stuff is precisely the core of the logic in join. So if you see a way to optimize the computation of the super categories of C.MyAxiom() while preserving the above feature, then I believe you actually have found a way to optimize join in the first place.

Cheers, Nicolas

PS: let's keep in mind this idea of using the reduction. It could indeed be that it could be used in a place or two to simplify the logic.