Fix element and parent classes of Hom categories to be abstract, and simplify the Hom logic.

[nthiery]

This patch fixes the parent and element classes for Hom categories to
be purely abstract, and simplifies the Hom logic:

• Unified the logic for selecting the class when building a Homset
  (e.g. Homset, RingHomset, HeckeModuleHomspace, ...). This is now
  systematically done through the _Hom_ hook. The logic still has a
  fundamental flaw, but that's for the later Refactor category support for morphisms (Hom is not a functorial construction!) #10668.
• The cache for Hom is handled at a single point in Hom
  In particular, homsets created via the _Hom_ hook are now unique.
• If category is None, Hom simply calls itself with the meet of the
  categories of the parent, which removes a cache handling duplication
  in the code
• Parent.Hom calls Hom directly (removes duplicate _Hom_ logic).
• ParentWithBase.Hom was redundant and is gone.
• Reduce the footprint of the current trick to delegate
  Hom(F,F)(on_basis=...) to module_morphism, allow for the diagonal
  option too, an make sure the homset category is set properly.
• Update a doctest in sage.modules.vector_space_homspace to take into
  account that homsets created via _Hom_ are now unique.
• Scheme is (apparently) an abstract base class; so it should not be
  instantiated. I changed some doctests in
  sage.schemes.generic.SchemeMorphism to use instead the concrete
  Spec(ZZ). Those doctests were breaking because Scheme does not
  implement equality, which is required for Hom caching.

As a byproduct, the HeckeModules category does not import any more
HeckeModulesHomspace, which was a recurrent source of import loops.

#11935 depends on this ticket

Apply:

• attachment: trac_12876_category_abstract_classes_for_hom.patch

Depends on #715
Depends on #11521
Depends on #12215
Depends on #12313
Depends on #13412
Depends on #13145
Depends on #14159
Depends on #13184
Depends on #14287
Depends on #14217

CC: @sagetrac-sage-combinat @simon-king-jena

Component: categories

Keywords: Hom

Author: Nicolas M. Thiéry

Reviewer: Simon King

Merged: sage-5.11.beta0

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

[nthiery]

comment:1

Note: this might depend on #12875

[nthiery]

Reviewer: Simon King

[nthiery]

comment:3

Note: there are two doctests failures that I don't know how to handle, related to the refcounting of Singular rings::

sage -t  devel/sage-combinat/sage/rings/polynomial/multi_polynomial_libsingular.pyx
**********************************************************************
File "/opt/sage-5.0.beta11/devel/sage-combinat/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 418:
    sage: len(ring_refcount_dict) == n
Expected:
    True
Got:
    False
sage -t  devel/sage-combinat/sage/libs/singular/ring.pyx
**********************************************************************
File "/opt/sage-5.0.beta11/devel/sage-combinat/sage/libs/singular/ring.pyx", line 469:
    sage: ring_ptr in ring_refcount_dict
Expected:
    False
Got:
    True

Help fixing those welcome!

[nthiery]

comment:4

The updated patch includes two small related improvements I had in later patches. I moved them here to resolve the conflict.

[nthiery]

comment:6

Oops. The updated patch includes two further hunks I had forgotten.

[nthiery]

comment:7

Arr, yet another missing hunk ... Time to go to bed!

[nthiery]

Dependencies: #12877

[nthiery]

comment:8

For the record, all test passed on 5.0.beta13, with #12875 and #12877 applied (and a couple unrelated ones). I expect no dependency upon #12875, but can't swear at this point.

[nthiery]

Changed dependencies from #12877 to #12875, #12877

[nthiery]

comment:10

Replying to @nthiery:

I expect no dependency upon #12875, but can't swear at this point.

Actually it does depend on #12875.

[simon-king-jena]

Work Issues: Fix ring_refcount_dict problem

[simon-king-jena]

comment:11

With #12808, #12875 and #12877 applied on top of sage-5.1.notebook, all doctests pass. But when adding the patch from here, there are two problems:

	sage -t  -force_lib "devel/sage/sage/libs/singular/ring.pyx"
	sage -t  -force_lib "devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx"

Namely:

sage -t -force_lib "devel/sage/sage/libs/singular/ring.pyx" 
**********************************************************************
File "/mnt/local/king/SAGE/experiment/notebook/sage-5.1.notebook/devel/sage/sage/libs/singular/ring.pyx", line 469:
    sage: ring_ptr in ring_refcount_dict
Expected:
    False
Got:
    True
**********************************************************************

and

sage -t -force_lib "devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
**********************************************************************
File "/mnt/local/king/SAGE/experiment/notebook/sage-5.1.notebook/devel/sage/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 418:
    sage: len(ring_refcount_dict) == n
Expected:
    True
Got:
    False
**********************************************************************

So, apparently it is only a single problem.

[simon-king-jena]

comment:12

PS: I just verified that the problem also occurs without #12808.

[nthiery]

comment:13

Replying to @simon-king-jena:

With #12808, #12875 and #12877 applied on top of sage-5.1.notebook, all doctests pass. But when adding the patch from here, there are two problems:

Yeah, I know (see comment 3 above). I feel quite stuck with those
failures, and would really appreciate if you could investigate them
since you know much better than me the caching for Singular rings.

Thanks!

Cheers,
Nicolas

[simon-king-jena]

comment:14

In one of my "weak caching" patches (see #11521), I also had to fix a ring_refcount_dict test. Thus, I tried to see whether applying #11521 in addition to your patch helps. Unfortunately there is an incompatibility, so, it is not resolved yet.

[simon-king-jena]

comment:15

Since there is a positive review for #11521 and #715 (which belong together and have otherwise no pending dependency), could you consider to rebase your patch relative to #11521?

[simon-king-jena]

comment:16

PS: I am just attempting a rebase, to see whether #11521 fixes the ring_refcount_dict problem.

[simon-king-jena]

comment:17

Here is the rejection:

--- homset.py
+++ homset.py
@@ -165,43 +203,27 @@ def Hom(X, Y, category=None):
             if H.domain() is X and H.codomain() is Y:
                 return H
 
-    try:
-        return X._Hom_(Y, category)
-    except (AttributeError, TypeError):
-        pass
-
     cat_X = X.category()
     cat_Y = Y.category()
     if category is None:
-        category = cat_X._meet_(cat_Y)
-    elif isinstance(category, Category):
-        if not cat_X.is_subcategory(category):
-            raise TypeError, "%s is not in %s"%(X, category)
-        if not cat_Y.is_subcategory(category):
-            raise TypeError, "%s is not in %s"%(Y, category)
-    else:
+        return Hom(X,Y,category=cat_X._meet_(cat_Y))
+    if not isinstance(category, Category):
         raise TypeError, "Argument category (= %s) must be a category."%category
-    # Now, as the category may have changed, we try to find the hom set in the cache, again:
-    key = (X,Y,category)
-    if _cache.has_key(key):
-        H = _cache[key]()
-        if H:
-            # Are domain or codomain breaking the unique parent condition?
-            if H.domain() is X and H.codomain() is Y:
-                return H
-
-    # coercing would be incredibly annoying, since the domain and codomain
-    # are totally different objects
-    #X = category(X); Y = category(Y)
+    if not cat_X.is_subcategory(category):
+        raise TypeError, "%s is not in %s"%(X, category)
+    if not cat_Y.is_subcategory(category):
+        raise TypeError, "%s is not in %s"%(Y, category)
 
     # construct H
     # Design question: should the Homset classes get the category or the homset category?
     # For the moment, this is the category, for compatibility with the current implementations
     # of Homset in rings, schemes, ...
-    H = category.hom_category().parent_class(X, Y, category = category)
-            
-    ##_cache[key] = weakref.ref(H)
-    _cache[(X, Y, category)] = weakref.ref(H)
+    from sets_cat import Sets
+    try:
+        H = X._Hom_(Y, category)
+    except (AttributeError, TypeError):
+        H = Homset(X, Y, category = category)
+    _cache[key] = weakref.ref(H)
     return H
 
 def hom(X, Y, f):

Why do you import Sets (new line 221)?

Do I understand correctly: If Hom receives None as category, then you determine the category as the meet of the categories of domain and codomain, and call Hom again. Did you test that the calling overhead does not matter?

[simon-king-jena]

comment:18

Another point: I had inserted a comment in #11521, namely "Apparently _Hom_ is supposed to be cached." This is why I did not insert stuff in the (weak) cache.

Do you lift that assumption? Then, we should see what _Hom_ methods actually do caching. They should at most do weak caching.

[simon-king-jena]

comment:148

You should also tell the patchbot what to do.

Apply trac_12876_category_abstract_classes_for_hom.patch

[nthiery]

comment:149

Replying to @simon-king-jena:

BTW, thank you for fixing the coercion cache of InfinitePolynomialRing. Now, as the default cache is weak anyway, the custom cache can certainly be removed now.

Well, actually you wrote this piece; see [comment:59]!

I am fine with this change. And since you had forgotten that you had
written it in the first place, I consider your self-review of this
piece as sufficient :-)

[nthiery]

comment:150

Replying to @simon-king-jena:

You should also tell the patchbot what to do.

Apply trac_12876_category_abstract_classes_for_hom.patch

Ah shoot; I had done this in the description but not here. Thanks. I really don't like that the patch bot is extracting this info from the comments rather than the description. Oh well.

[nthiery]

comment:151

All test passed:

http://sage.math.washington.edu/home/nthiery/trac_12876_category_abstract_classes_for_hom.patch-testlog

Well, almost. I got the following error:

sage -t --long devel/sage/sage/graphs/genus.pyx
**********************************************************************
File "devel/sage/sage/graphs/genus.pyx", line 136, in sage.graphs.genus.simple_connected_genus_backtracker.__dealloc__
Failed example:
    get_memory_usage(t) <= 0.0
Expected:
    True
Got:
    False

but can't reproduce it; so this sounds more like an unrelated random sporadic unfluke.

Let's see what the patchbot says!

[nthiery]

comment:152

It seems to be happy :-)

[simon-king-jena]

comment:153

OK, I am just waiting for my own tests to finish, and will then provide a reviewer patch, adding a test that shows that providing Category.meet([X.category(),Y.category()]) is equivalent to providing None.

[simon-king-jena]

Attachment: trac_12876-review.patch.gz

[simon-king-jena]

comment:154

Or better: Add the patch now and let the bot start over.

Apply trac_12876_category_abstract_classes_for_hom.patch trac_12876-review.patch

[nbruin]

comment:155

for the review patch: "identic" -> "identical"

[nthiery]

comment:156

Replying to @nbruin:

for the review patch: "identic" -> "identical"

Yup. I am on it.

[nthiery]

Attachment: trac_12876_category_abstract_classes_for_hom.patch.gz

Combined patch

[nthiery]

comment:158

I folded in you review patch, and fixed identic to identical (also in a paragraph just after).

Apply:

• attachment: trac_12876_category_abstract_classes_for_hom.patch

[simon-king-jena]

comment:159

OK, for me, the tests pass. So, I make it a positive review.

[simon-king-jena]

comment:160

PS, I did briefly check the combined patch...

Apply trac_12876_category_abstract_classes_for_hom.patch

[nthiery]

comment:161

Youpi!

Thanks Simon!

[jdemeyer]

Merged: sage-5.11.beta0