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 #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.
• 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:

• trac_12876_category-fix_abstract_class-nt-rel11521.patch​
• trac_12876-reviewer.patch​

[nthiery]

Note: this might depend on #12875

[nthiery]

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]

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

[nthiery]

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

[nthiery]

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

[nthiery]

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]

Replying to nthiery:

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

Actually it does depend on #12875.

[SimonKing]

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.

[SimonKing]

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

[nthiery]

Replying to SimonKing:

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

[SimonKing]

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.

[SimonKing]

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?

[SimonKing]

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

[SimonKing]

Here is the rejection:

homset.py

@@ -165 +203 @@
             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?

[SimonKing]

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.

[SimonKing]

Nicolas' patch rebased rel #11521

[SimonKing]

I have rebased your patch relative to #11521 (as I said: This already has positive review).

Please have a look whether you are happy with my changes: I only changed sage/categories/homset.py, by using the :trac: directive in two places, fixing one hunk that has only applied with fuzz and removing the import of Sets. Apart from that, I adopted your logic of constructing the homset.

So far, I have tested sage/categories/homset.py and have verified that the previously failing tests are indeed fixed by #11521 (in fact, I also had some other pathes applied, but I am convinced that ##11521 suffices). I am now running the full tests suite, with only #11521 (and its dependency #715) and the new patch (and its dependencies #12875 and #12877).

Apply trac_12876_category-fix_abstract_class-nt-rel11521.patch

[SimonKing]

Hm. With sage-5.0.beta13 plus #715, #11521, #12875, #12877 and trac_12876_category-fix_abstract_class-nt-rel11521.patch​, I get one error, namely a segfault in

sage -t -force_lib "devel/sage/sage/rings/number_field/number_field_rel.py"

But strange enough: When I ran it with --verbose, all tests passed, and the segfault came from leaving sage. That reminds me another problem: The deallocation of the (unique) pari instance is not done as it is supposed to be. This is fixed by the second patch of #12215. Hence, I am trying again, with #12215 added.

[SimonKing]

Yes! Adding #12215 fixes the segfault. I am now running doctests. Do you mind adding it as another dependency, provided that the test suite passes?

[SimonKing]

With sage-5.0.beta13 plus #715, #11521, #12875, #12877, #12215 and trac_12876_category-fix_abstract_class-nt-rel11521.patch​, all doc tests pass.

I made some additional cosmetic changes in a reviewer patch.

Further questions:

In sage/category/hecke_modules.py, you refer to ticket number ??? for fixing _test_zero and _test_elements. In sage/schemes/elliptic_curves/ell_curve_isogeny.py, you leave a reference to trac open as well. Have the tickets now been created? I didn't fix that yet.

I see that you rename Rings.HomCategory.ParentMethods.__new__ into __new__bx. Does that mean the ugly __new__ method ought to be removed (finally!)? But then, shouldn't __getnewargs__ be removed as well?

Please tell whether you want to address these questions in an additional patch. And I hope you agree with adding #715, #11521 and #12215 to the dependencies? Only #12215 needs review (hint...).

If you do agree, then put this to positive review, please.

Apply trac_12876_category-fix_abstract_class-nt-rel11521.patch trac_12876-reviewer.patch