Opened 8 years ago

Closed 6 years ago

#12876 closed enhancement (fixed)

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

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-5.11
Component: categories Keywords: categories, Hom
Cc: sage-combinat, SimonKing Merged in: sage-5.11.beta0
Authors: Nicolas M. Thiéry Reviewers: Simon King
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #715, #11521, #12215, #12313, #13412, #13145, #14159, #13184, #14287, #14217 Stopgaps:

Description (last modified by 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 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:

Attachments (11)

trac_12876_category-fix_abstract_class-nt-rel11521.patch (34.8 KB) - added by SimonKing 8 years ago.
Nicolas' patch rebased rel #11521
trac_12876-reviewer.patch (5.8 KB) - added by SimonKing 8 years ago.
trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch (4.4 KB) - added by nthiery 8 years ago.
trac_12876_category-fix_abstract_class-sk-rel11521.patch (34.8 KB) - added by SimonKing 7 years ago.
Replacement of trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch wrt to the new patch versions at #715, #12313 etc.
trac_12876_fix_infinite_polynomial_ring.patch (3.3 KB) - added by SimonKing 7 years ago.
Do not weak-cache things in infinite_polynomial_ring that are cached anyway. Skip more tests in Hecke homset's TestSuite
trac_12876_another_reviewer.patch (920 bytes) - added by SimonKing 7 years ago.
Another reviewer patch
trac_12876-schemes_homcategory.patch (2.6 KB) - added by nthiery 6 years ago.
trac_12876_r_test.patch (1.1 KB) - added by SimonKing 6 years ago.
Try to make one test in interfaces/r.py more stable wrt symbolic links
trac_12876-fix-homset-cache-nt.patch (3.7 KB) - added by nthiery 6 years ago.
trac_12876-review.patch (908 bytes) - added by SimonKing 6 years ago.
trac_12876_category_abstract_classes_for_hom.patch (46.2 KB) - added by nthiery 6 years ago.
Combined patch

Download all attachments as: .zip

Change History (174)

comment:1 Changed 8 years ago by nthiery

  • Status changed from new to needs_review

Note: this might depend on #12875

comment:2 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:3 Changed 8 years ago by nthiery

  • Reviewers set to Simon King

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!

comment:4 Changed 8 years ago by nthiery

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

comment:5 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:6 Changed 8 years ago by nthiery

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

comment:7 Changed 8 years ago by nthiery

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

comment:8 follow-up: Changed 8 years ago by nthiery

  • Dependencies set to #12877

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.

comment:9 Changed 8 years ago by nthiery

  • Cc SimonKing added

comment:10 in reply to: ↑ 8 Changed 8 years ago by nthiery

  • Dependencies changed from #12877 to #12875, #12877

Replying to nthiery:

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

Actually it does depend on #12875.

comment:11 follow-up: Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Fix ring_refcount_dict problem

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.

comment:12 Changed 8 years ago by SimonKing

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

comment:13 in reply to: ↑ 11 Changed 8 years ago by 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

comment:14 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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?

comment:16 Changed 8 years ago by SimonKing

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

comment:17 follow-up: Changed 8 years ago by SimonKing

Here is the rejection:

  • homset.py

    def Hom(X, Y, category=None): 
    165203            if H.domain() is X and H.codomain() is Y:
    166204                return H
    167205
    168     try:
    169         return X._Hom_(Y, category)
    170     except (AttributeError, TypeError):
    171         pass
    172 
    173206    cat_X = X.category()
    174207    cat_Y = Y.category()
    175208    if category is None:
    176         category = cat_X._meet_(cat_Y)
    177     elif isinstance(category, Category):
    178         if not cat_X.is_subcategory(category):
    179             raise TypeError, "%s is not in %s"%(X, category)
    180         if not cat_Y.is_subcategory(category):
    181             raise TypeError, "%s is not in %s"%(Y, category)
    182     else:
     209        return Hom(X,Y,category=cat_X._meet_(cat_Y))
     210    if not isinstance(category, Category):
    183211        raise TypeError, "Argument category (= %s) must be a category."%category
    184     # Now, as the category may have changed, we try to find the hom set in the cache, again:
    185     key = (X,Y,category)
    186     if _cache.has_key(key):
    187         H = _cache[key]()
    188         if H:
    189             # Are domain or codomain breaking the unique parent condition?
    190             if H.domain() is X and H.codomain() is Y:
    191                 return H
    192 
    193     # coercing would be incredibly annoying, since the domain and codomain
    194     # are totally different objects
    195     #X = category(X); Y = category(Y)
     212    if not cat_X.is_subcategory(category):
     213        raise TypeError, "%s is not in %s"%(X, category)
     214    if not cat_Y.is_subcategory(category):
     215        raise TypeError, "%s is not in %s"%(Y, category)
    196216
    197217    # construct H
    198218    # Design question: should the Homset classes get the category or the homset category?
    199219    # For the moment, this is the category, for compatibility with the current implementations
    200220    # of Homset in rings, schemes, ...
    201     H = category.hom_category().parent_class(X, Y, category = category)
    202            
    203     ##_cache[key] = weakref.ref(H)
    204     _cache[(X, Y, category)] = weakref.ref(H)
     221    from sets_cat import Sets
     222    try:
     223        H = X._Hom_(Y, category)
     224    except (AttributeError, TypeError):
     225        H = Homset(X, Y, category = category)
     226    _cache[key] = weakref.ref(H)
    205227    return H
    206228
    207229def 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?

comment:18 follow-up: Changed 8 years ago by 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.

Changed 8 years ago by SimonKing

Nicolas' patch rebased rel #11521

comment:19 Changed 8 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

comment:20 Changed 8 years ago by 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.

comment:21 follow-up: Changed 8 years ago by 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?

comment:22 follow-up: Changed 8 years ago by SimonKing

  • Dependencies changed from #12875, #12877 to #715, #11521, #12875, #12877, #12215
  • Description modified (diff)

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

Changed 8 years ago by SimonKing

comment:23 Changed 8 years ago by SimonKing

Sorry, I made a last-minute change in the reviewer patch (namely, by mistake, I changed "ticket ???" into "ticket 12876", but I think ??? really refers to a ticket that has not been created, yet. Anyway, feel free to change ??? into something real! And disregard trac_12876-reviewer.2.patch, which was created by mistake.

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

comment:24 Changed 8 years ago by SimonKing

I tried whether one can remove the __new__ and __getnewargs__ methods. However, when doing so, one gets a reproducible timeout in the tests for sage/rings/polynomial/multi_polynomial_sequence.py - that's the only error.

So, better keep it as it is.

Recall: I would give it a positive review, if you agree with the additional dependencies.

comment:25 in reply to: ↑ 17 Changed 8 years ago by nthiery

Replying to SimonKing:

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?

I did not test it. But it simplifies quite much the logic and the calling overhead occurs only for the first call, so I assumed it was negligible.

comment:26 in reply to: ↑ 21 Changed 8 years ago by nthiery

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

That's ok for me. Thanks for tracking all of those down! I'll try to review #12215, but that probably won't be before Monday.

comment:27 in reply to: ↑ 22 Changed 8 years ago by nthiery

Hi Simon!

Replying to 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.

Cool!

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.

Oops. Those are #12879 and #12880 respectively.

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?

Yes, that was my intention indeed: in principle they should not be needed, and I forgot to remove them. I'll investigate the failure you report on Monday, unless you beat me to it.

Please tell whether you want to address these questions in an additional patch.

Unless removing getnewargs turns out to be hard, I'd rather fix all of the above right now. Feel free to experiment: I won't download the patches before Monday.

Thanks a lot for all your quick work on this :-)

comment:28 in reply to: ↑ 18 Changed 8 years ago by nthiery

Replying to 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.

Yes: I consider that all the caching logic should be in Hom; in particular the _Hom_ methods should not do caching (and if I recall correctly, they currently don't). We should probably add a note about this in the documentation of Hom.

comment:29 Changed 8 years ago by nthiery

I have uploaded a reviewer's patch which fixes the trac ticket numbers, and removes completely Rings.HomCategory?. I could not reproduce the timeout:

sage -t  "devel/sage-combinat/sage/rings/polynomial/multi_polynomial_sequence.py"
	 [8.6 s]

Can you check again?

If you are happy with the change, please fold everything, and set a positive review.

comment:30 Changed 8 years ago by SimonKing

I just verified: I don't see a timeout anymore. Strange.

Anyway, I am now running make testlong, and if everything works, I'll give it a positive review and provide a combined patch.

comment:31 follow-up: Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work

Too bad. "make testlong" resulted in

sage -t  --long -force_lib "devel/sage/sage/categories/map.pyx"
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/map.pyx", line 382:
    sage: phi.category()
Expected:
    Category of hom sets in Category of rings
Got:
    Category of hom sets in Category of sets
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/map.pyx", line 388:
    sage: f.category()
Expected:
    Join of Category of hom sets in Category of rings and Category of hom sets in Category of modules over Rational Field
Got:
    Category of hom sets in Category of modules over Rational Field
**********************************************************************

and

sage -t  --long -force_lib "devel/sage/sage/categories/homset.py"
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/homset.py", line 228:
    sage: Hom(PA,PJ).category()
Expected:
    Join of Category of hom sets in Category of rings and Category of hom sets in Category of modules over Rational Field
Got:
    Category of hom sets in Category of modules over Rational Field
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/homset.py", line 339:
    sage: End(QQ).category()
Expected:
    Category of hom sets in Category of rings
Got:
    Category of hom sets in Category of sets

I have sage-5.0.beta13 plus #715, #11521, #12875, #12877, #12215 and #12876.

comment:32 in reply to: ↑ 31 Changed 8 years ago by SimonKing

Replying to SimonKing:

I have sage-5.0.beta13 plus #715, #11521, #12875, #12877, #12215 and #12876.

To be precise (in case I forgot to apply one patch):

king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage$ hg qa
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12875-category-fix_abvar_homspace-nt.patch
trac_12877-category-for_more_rings_and_schemes-nt.patch
trac12215_weak_cached_function.patch
trac12215_segfault_fixes.patch
trac_12876_category-fix_abstract_class-nt-rel11521.patch
trac_12876-reviewer.patch
trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch

comment:33 Changed 8 years ago by nthiery

Ok, this is totally benign. Now, to minimize change, the easiest is probably to keep the HomCategory? even if we delete its containt. The updated reviewer's patch does just that.

Cheers,

Nicolas

comment:34 follow-up: Changed 8 years ago by SimonKing

  • Work issues changed from Fix ring_refcount_dict problem to Fix the review-nt patch

trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch doesn't work, because you confused ` with '.

Please change the following hunk:

  • hecke_modules.py

    class HeckeModules(Category_module): 
    116116            Fixing :meth:`_test_zero` (``__call__`` should accept a
    117117            function as input) and :meth:`_test_elements` (modular
    118118            form morphisms elements should inherit from categories) is
    119             :trac:`???`.
     119            :trac:`12879`.
    120120
    121121            TESTS::

since the to-be-changed file has :trac:'???', hence, no backticks around ???.

comment:35 follow-up: Changed 8 years ago by SimonKing

  • Work issues changed from Fix the review-nt patch to Fix the review-nt patch, fix doctests in map.pyx and homset.py

I get four errors with make ptestlong:

----------------------------------------------------------------------

The following tests failed:

	sage -t  --long -force_lib devel/sage/sage/categories/homset.py # 2 doctests failed
	sage -t  --long -force_lib devel/sage/sage/categories/map.pyx # 2 doctests failed
----------------------------------------------------------------------

namely

sage -t --long -force_lib "devel/sage/sage/categories/homset.py"
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/homset.py", line 228:
    sage: Hom(PA,PJ).category()
Expected:
    Join of Category of hom sets in Category of rings and Category of hom sets in Category of modules over Rational Field
Got:
    Category of hom sets in Category of modules over Rational Field
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/homset.py", line 337:
    sage: End(QQ).category()
Expected:
    Category of hom sets in Category of rings
Got:
    Category of hom sets in Category of sets
**********************************************************************

and

sage -t --long -force_lib "devel/sage/sage/categories/map.pyx"
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/map.pyx", line 382:
    sage: phi.category()
Expected:
    Category of hom sets in Category of rings
Got:
    Category of hom sets in Category of sets
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/map.pyx", line 388:
    sage: f.category()
Expected:
    Join of Category of hom sets in Category of rings and Category of hom sets in Category of modules over Rational Field
Got:
    Category of hom sets in Category of modules over Rational Field
**********************************************************************

For reference, I have

king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage$ hg qapplied 
trac_12808-classcall_speedup-fh.patch
trac_12808_nested_class_cython.patch
trac_12808-classcall_cdef.patch
trac12215_weak_cached_function.patch
trac12215_segfault_fixes.patch
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12875-category-fix_abvar_homspace-nt.patch
trac_12877-category-for_more_rings_and_schemes-nt.patch
trac_12876_category-fix_abstract_class-nt-rel11521.patch
trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch

How is that different from your setting?

comment:36 in reply to: ↑ 34 ; follow-up: Changed 8 years ago by nthiery

Replying to SimonKing:

trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch doesn't work, because you confused ` with '.

Please change the following hunk:

  • hecke_modules.py

    class HeckeModules(Category_module): 
    116116            Fixing :meth:`_test_zero` (``__call__`` should accept a
    117117            function as input) and :meth:`_test_elements` (modular
    118118            form morphisms elements should inherit from categories) is
    119             :trac:`???`.
     119            :trac:`12879`.
    120120
    121121            TESTS::

since the to-be-changed file has :trac:'???', hence, no backticks around ???.

Did you include your trac_12876-reviewer.patch (which fixes those backticks) in between trac_12876_category-fix_abstract_class-nt-rel11521.patch and trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch?

comment:37 in reply to: ↑ 35 ; follow-up: Changed 8 years ago by nthiery

Replying to SimonKing:

I get four errors with make ptestlong: ...

sage -t --long -force_lib "devel/sage/sage/categories/map.pyx"
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/map.pyx", line 382:
    sage: phi.category()
Expected:
    Category of hom sets in Category of rings
Got:
    Category of hom sets in Category of sets
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/categories/map.pyx", line 388:
    sage: f.category()
Expected:
    Join of Category of hom sets in Category of rings and Category of hom sets in Category of modules over Rational Field
Got:
    Category of hom sets in Category of modules over Rational Field
**********************************************************************

This looks like the errors I was getting when I had removed completely Rings.HomCategory?. Can you double check that you have the latest version of the reviewer patch from trac and that categories/rings.py indeed has an (empty) class Rings.Homcategory?

comment:38 in reply to: ↑ 36 Changed 8 years ago by SimonKing

Replying to nthiery:

Did you include your trac_12876-reviewer.patch (which fixes those backticks) in between trac_12876_category-fix_abstract_class-nt-rel11521.patch and trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch?

Nope! I thought that your review patch replaces mine.

comment:39 in reply to: ↑ 37 Changed 8 years ago by SimonKing

Replying to nthiery:

This looks like the errors I was getting when I had removed completely Rings.HomCategory?. Can you double check that you have the latest version of the reviewer patch from trac and that categories/rings.py indeed has an (empty) class Rings.Homcategory?

No, categories/rings.py has no Homcategory. So, am I using the wrong patch?

comment:40 Changed 8 years ago by SimonKing

  • Description modified (diff)
  • Work issues changed from Fix the review-nt patch, fix doctests in map.pyx and homset.py to Fx doctests in map.pyx and homset.py

Application works when both reviewer patches are applied. I am changing the ticket description accordingly.

comment:41 follow-up: Changed 8 years ago by SimonKing

PS: With

king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage$ hg qa
trac_12808-classcall_speedup-fh.patch
trac_12808_nested_class_cython.patch
trac_12808-classcall_cdef.patch
trac12215_weak_cached_function.patch
trac12215_segfault_fixes.patch
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12875-category-fix_abvar_homspace-nt.patch
trac_12877-category-for_more_rings_and_schemes-nt.patch
trac_12876_category-fix_abstract_class-nt-rel11521.patch
trac_12876-reviewer.patch
trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch

sage.categories.rings.Rings.HomCategory exists and is empty, as it is supposed to be, I guess.

comment:42 follow-up: Changed 8 years ago by SimonKing

PS: We really should do something about source code inspection! See what happens if you do

sage: sage.categories.rings.Rings.HomCategory??

or

sage: edit(sage.categories.rings.Rings.HomCategory,'vim')

I thought I had fixed that in some ticket!!

comment:43 in reply to: ↑ 42 Changed 8 years ago by SimonKing

Replying to SimonKing:

I thought I had fixed that in some ticket!!

I did: #11768, needing review (hint-hint).

comment:44 in reply to: ↑ 41 ; follow-up: Changed 8 years ago by nthiery

Replying to SimonKing:

PS: With

king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage$ hg qa
trac_12808-classcall_speedup-fh.patch
trac_12808_nested_class_cython.patch
trac_12808-classcall_cdef.patch
trac12215_weak_cached_function.patch
trac12215_segfault_fixes.patch
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12875-category-fix_abvar_homspace-nt.patch
trac_12877-category-for_more_rings_and_schemes-nt.patch
trac_12876_category-fix_abstract_class-nt-rel11521.patch
trac_12876-reviewer.patch
trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch

sage.categories.rings.Rings.HomCategory exists and is empty, as it is supposed to be, I guess.

Cool. I guess you are running the tests now, right?

comment:45 in reply to: ↑ 44 Changed 8 years ago by SimonKing

Replying to nthiery:

Cool. I guess you are running the tests now, right?

I have already verified that the two problematic tests pass. If it works, it will be a positive review.

Admittedly, I am running the test suite with #9107 and #11768 applied as well...

comment:46 Changed 8 years ago by SimonKing

  • Work issues changed from Fx doctests in map.pyx and homset.py to Fx doctests in sageinspect.py

I'm afraid it still needs a tiny bit of work. make ptestlong resulted in (only) one error, namely

sage -t --long -force_lib "devel/sage/sage/misc/sageinspect.py"
**********************************************************************
File "/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage/sage/misc/sageinspect.py", line 1495:
    sage: sage_getsourcelines(HC)
Expected:
    (['    class HomCategory(HomCategory):\n',
     '        def extra_super_categories(self):\n',
     ...
     '                return (self.domain(), self.codomain(), self.category())\n'], ...)
Got:
    (['    class HomCategory(HomCategory):\n', '        pass\n'], 564)
**********************************************************************
1 items had failures:
   1 of  24 in __main__.example_27
***Test Failed*** 1 failures.
For whitespace errors, see the file /mnt/local/king/.sage/tmp/sageinspect_13253.py
         [14.0 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t --long -force_lib "devel/sage/sage/misc/sageinspect.py"
Total time for all tests: 14.0 seconds

Apparently this is caused by emptying the HomCategory of Rings().

comment:47 Changed 8 years ago by SimonKing

PS: The obvious fix is to change the expected result in the test...

comment:48 Changed 8 years ago by SimonKing

Me stupid! The failing test is only introduced in #11768. So, I had better not apply more than the necessary patches...

Running the failing tests without the additional patch works.

Hence, repeating the test suite now... Sorry!

comment:49 follow-up: Changed 8 years ago by SimonKing

  • Status changed from needs_work to positive_review

Now it works. It is thus a positive review.

comment:50 in reply to: ↑ 49 Changed 8 years ago by nthiery

Replying to SimonKing:

Now it works. It is thus a positive review.

Thanks! I can't wait for 5.1 :-)

comment:51 Changed 8 years ago by SimonKing

  • Work issues Fx doctests in sageinspect.py deleted

I forgot to remove the "work issues".

comment:52 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-pending

comment:53 Changed 8 years ago by jdemeyer

  • Work issues set to commit message

The third patch needs a proper commit message.

comment:54 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.4
  • Status changed from positive_review to needs_work

comment:55 Changed 7 years ago by jdemeyer

  • Dependencies changed from #715, #11521, #12875, #12877, #12215 to #715, #11521, #12215

This needs to be rebased to sage-5.3.rc0 + #715 + #11521 + #12215 + #12313.

comment:56 Changed 7 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I have replaced Nicolas' first patch, under a new name. The changes became necessary by changes in the patches at #715 and #12313. Now I am running doctests.

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

comment:57 Changed 7 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues changed from commit message to fix doctests and segfaults

With sage-5.3.b2 plus #715, #11521, #12215, #12313, #13370, #13378 and the patches from here, I get

make ptest
...
The following tests failed:

        sage -t  -force_lib devel/sage/sage/interfaces/r.py # 1 doctests failed
        sage -t  -force_lib devel/sage/sage/rings/polynomial/infinite_polynomial_ring.py # 0 doctests failed
        sage -t  -force_lib devel/sage/sage/categories/hecke_modules.py # 1 doctests failed

The error in hecke_modules.py is due to

NotImplementedError: please implement _an_element_ for Set of Morphisms from Modular Forms space of dimension 3 for Congruence Subgroup Gamma0(7) of weight 4 over Rational Field to Modular Forms space of dimension 3 for Congruence Subgroup Gamma0(7) of weight 4 over Rational Field in Category of Hecke modules over Rational Field

The error in expect/r.py looks strange to me:

sage -t  -force_lib devel/sage/sage/interfaces/r.py
**********************************************************************
File "/mnt/local/king/SAGE/prereleases/sage-5.3.beta2/devel/sage-main/sage/interfaces/r.py", line 1169:
    sage: os.path.realpath(tmpdir) == sageobj(r.getwd())
Expected:
    True
Got:
    False
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_43

The error is not reproducible, at least not if I run the test individually.

In infinite_polynomial_ring.py one gets a segfault. One does not get a segfault when running the tests with --verbose. But gdb yields the following backtrace:

#0  PyObject_Malloc (nbytes=123) at Objects/obmalloc.c:788
#1  0x00007ffff7a9bd5b in string_concat (a=0x7ffff7ed6730, bb=0x54c6180) at Objects/stringobject.c:1056
#2  0x00007ffff7a9dda5 in PyString_Concat (pv=0x7fffffffac58, w=<value optimized out>) at Objects/stringobject.c:3862
#3  0x00007ffff7a46025 in string_concatenate (v=0x7ffff7ed6730, w=0x54c6180, f=<value optimized out>, next_instr=<value optimized out>) at Python/ceval.c:4856
#4  0x00007ffff7af56c7 in PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:1548
#5  0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=2, kws=0x49a5b70, kwcount=0,
    defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3253
#6  0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#7  call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#8  PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#9  0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=2, kws=0x49a5930, kwcount=0,
    defs=0x7ffff7f70968, defcount=1, closure=0x0) at Python/ceval.c:3253
#10 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#11 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#12 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#13 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=2, kws=0x50ed188, kwcount=0,
    defs=0x7ffff7f708e8, defcount=1, closure=0x0) at Python/ceval.c:3253
#14 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#15 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#16 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#17 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=3, kws=0x50ecf90, kwcount=0,
    defs=0x4918628, defcount=1, closure=0x0) at Python/ceval.c:3253
#18 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#19 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#20 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#21 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=3, kws=0x50ecd88, kwcount=0,
    defs=0x7ffff7f708a8, defcount=1, closure=0x0) at Python/ceval.c:3253
#22 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#23 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#24 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#25 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=3, kws=0x50ec980, kwcount=0, 
    defs=0x7ffff7f5aa40, defcount=2, closure=0x0) at Python/ceval.c:3253
#26 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#27 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#28 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#29 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=3, kws=0x50eac40, kwcount=1, 
    defs=0x7ffff7ee9218, defcount=2, closure=0x0) at Python/ceval.c:3253
#30 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#31 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#32 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#33 0x00007ffff7af627b in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4107
#34 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#35 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#36 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=4, kws=0x0, kwcount=0, 
    defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3253
#37 0x00007ffff7a7a02c in function_call (func=0x491cb18, arg=0x476c050, kw=0x0) at Objects/funcobject.c:526
#38 0x00007ffff7a522c3 in PyObject_Call (func=0x491cb18, arg=<value optimized out>, kw=<value optimized out>) at Objects/abstract.c:2529
#39 0x00007ffff7a5fa0f in instancemethod_call (func=0x491cb18, arg=0x476c050, kw=0x0) at Objects/classobject.c:2578
#40 0x00007ffff7a522c3 in PyObject_Call (func=0x42995f0, arg=<value optimized out>, kw=<value optimized out>) at Objects/abstract.c:2529
#41 0x00007ffff7af40fd in do_call (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4239
#42 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4044
#43 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#44 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=4, kws=0x4d12cf0, kwcount=0, 
    defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3253
#45 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#46 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#47 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#48 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=2, kws=0x488e5b0, kwcount=3, 
    defs=0x491d388, defcount=3, closure=0x0) at Python/ceval.c:3253
#49 0x00007ffff7a7a123 in function_call (func=0x491cc80, arg=0x4906cb0, kw=0x494f090) at Objects/funcobject.c:526
#50 0x00007ffff7a522c3 in PyObject_Call (func=0x491cc80, arg=<value optimized out>, kw=<value optimized out>) at Objects/abstract.c:2529
#51 0x00007ffff7a5fa0f in instancemethod_call (func=0x491cc80, arg=0x4906cb0, kw=0x494f090) at Objects/classobject.c:2578
#52 0x00007ffff7a522c3 in PyObject_Call (func=0x4856be0, arg=<value optimized out>, kw=<value optimized out>) at Objects/abstract.c:2529
#53 0x00007ffff7af40fd in do_call (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4239
#54 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4044
#55 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#56 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=2, kws=0x4068fd0, kwcount=0, 
    defs=0x48c48d8, defcount=3, closure=0x0) at Python/ceval.c:3253
#57 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#58 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#59 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#60 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=0, kws=0x49af830, kwcount=10, 
    defs=0x49104a8, defcount=10, closure=0x0) at Python/ceval.c:3253
#61 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#62 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#63 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#64 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=1, kws=0x6d8fb8, kwcount=3, 
    defs=0x4910310, defcount=10, closure=0x0) at Python/ceval.c:3253
#65 0x00007ffff7af5550 in fast_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4117
#66 call_function (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:4042
#67 PyEval_EvalFrameEx (f=<value optimized out>, throwflag=<value optimized out>) at Python/ceval.c:2666
#68 0x00007ffff7af7125 in PyEval_EvalCodeEx (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>, args=<value optimized out>, argcount=0, kws=0x0, kwcount=0, 
    defs=0x0, defcount=0, closure=0x0) at Python/ceval.c:3253
#69 0x00007ffff7af7262 in PyEval_EvalCode (co=<value optimized out>, globals=<value optimized out>, locals=<value optimized out>) at Python/ceval.c:667
#70 0x00007ffff7b19760 in run_mod (fp=0x6c66c0, filename=<value optimized out>, start=<value optimized out>, globals=0x640280, locals=0x640280, closeit=1, flags=0x7fffffffd380)
    at Python/pythonrun.c:1353
#71 PyRun_FileExFlags (fp=0x6c66c0, filename=<value optimized out>, start=<value optimized out>, globals=0x640280, locals=0x640280, closeit=1, flags=0x7fffffffd380) at Python/pythonrun.c:1339
#72 0x00007ffff7b1a1ff in PyRun_SimpleFileExFlags (fp=0x6c66c0, filename=0x7fffffffd8ff "/mnt/local/king/.sage/tmp/infinite_polynomial_ring_15153.py", closeit=1, flags=0x7fffffffd380)
    at Python/pythonrun.c:943
#73 0x00007ffff7b2d4b5 in Py_Main (argc=<value optimized out>, argv=<value optimized out>) at Modules/main.c:639
#74 0x00007ffff6e1ec8d in __libc_start_main (main=<value optimized out>, argc=<value optimized out>, ubp_av=<value optimized out>, init=<value optimized out>, fini=<value optimized out>, 
    rtld_fini=<value optimized out>, stack_end=0x7fffffffd498) at libc-start.c:228
#75 0x0000000000400619 in _start ()

Could very well be that this is (once again) due to the new weak caches. But I will also rerun without #13370 and #13378.

comment:58 Changed 7 years ago by jdemeyer

  • Dependencies changed from #715, #11521, #12215 to #715, #11521, #12215, #12313

comment:59 Changed 7 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Got it!

Infinite polynomial rings were using WeakKeyDict in _has_coerce_map_from_for caching whether there is a coercion. That has probably been a bad idea, because the methods that are really used (has_coerce_map) have a cache anyway.

Removing the additional cache fixes the segfault.

Apply trac_12876_category-fix_abstract_class-sk-rel11521.patch trac_12876-reviewer.patch trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch trac_12876_fix_infinite_polynomial_ring.patch

comment:60 follow-up: Changed 7 years ago by SimonKing

  • Dependencies changed from #715, #11521, #12215, #12313 to #11521, #12215, #12313

One has to remove the dependency #715, because #715 and #11521 are mutually dependent, but #715 needs to be applied first.

Anyway. There just remains the error in sage -t -force_lib devel/sage/sage/categories/hecke_modules.py, which amounts to writing a _an_element_ method. The error with r seems to have been noise.

comment:61 in reply to: ↑ 60 Changed 7 years ago by SimonKing

Replying to SimonKing:

Anyway. There just remains the error in sage -t -force_lib devel/sage/sage/categories/hecke_modules.py, which amounts to writing a _an_element_ method.

... or: One could replace that test - after all, the test is new (introduced in the first patch).

comment:62 follow-ups: Changed 7 years ago by SimonKing

I think implementing a _an_element_ is easy enough that I can do it in another reviewer patch. One thing doesn't work as described in the documentation, though. It says that a homomorphism of Hecke algebras can not only be obtained from a matrix (that works) but from an element of a Hecke algebra (I suppose: An element of the codomain). However, this isn't implemented.

I am not competent to implement it. So, I suggest that I start with implementing _an_element_, try to use the category framework on these homsets (i.e., define the Element attribute), and very important: Make the homsets unique upon pickling. Namely:

sage: M = ModularForms(Gamma0(7), 4)
sage: H = Hom(M,M)
sage: H is loads(dumps(H))
False

so that by consequence several tests from the test suite fail.

comment:63 in reply to: ↑ 62 Changed 7 years ago by SimonKing

Replying to SimonKing:

sage: M = ModularForms(Gamma0(7), 4)
sage: H = Hom(M,M)
sage: H is loads(dumps(H))
False

... which has two reasons: H has no __reduce__ method, and M has no reduce method.

comment:64 in reply to: ↑ 62 ; follow-up: Changed 7 years ago by nthiery

Hi Simon!

Replying to SimonKing:

I think implementing a _an_element_ is easy enough that I can do it in another reviewer patch. One thing doesn't work as described in the documentation, though. It says that a homomorphism of Hecke algebras can not only be obtained from a matrix (that works) but from an element of a Hecke algebra (I suppose: An element of the codomain). However, this isn't implemented.

I am not competent to implement it. So, I suggest that I start with implementing _an_element_, try to use the category framework on these homsets (i.e., define the Element attribute), and very important: Make the homsets unique upon pickling. Namely:

sage: M = ModularForms(Gamma0(7), 4)
sage: H = Hom(M,M)
sage: H is loads(dumps(H))
False

so that by consequence several tests from the test suite fail.

Feel free to just skip the failing tests and mention them, say on #12879.

And thanks a lot for all the work you put in rebasing/finalizing this patch! I'll try to look at it tomorrow.

Cheers,

Nicolas

comment:65 in reply to: ↑ 64 Changed 7 years ago by SimonKing

Replying to nthiery:

Feel free to just skip the failing tests and mention them, say on #12879.

... and to remove the erroneous documentation (or perhaps: Marking it as "TODO").

I was confident that I have produced a patch (not posted, though) that does the right thing with the categories of Hecke homsets. In fact, the full TestSuite of these homsets passes. However, a surprising amount of tests fail, all over sage. So, I guess you are right: One should work around for now, and fix it for real in #12879.

Cheers, Simon

comment:66 Changed 7 years ago by SimonKing

I have updated the last patch, so that more tests from the failing TestSuite are skipped. Let's see if the patchbot is happy.

Apply trac_12876_category-fix_abstract_class-sk-rel11521.patch trac_12876-reviewer.patch trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch trac_12876_fix_infinite_polynomial_ring.patch

Changed 7 years ago by SimonKing

Do not weak-cache things in infinite_polynomial_ring that are cached anyway. Skip more tests in Hecke homset's TestSuite

comment:67 Changed 7 years ago by SimonKing

Oops, I thought that I had an outdated version of my last patch, but apparently it has been the correct one. Anyway, I'll restart the patchbot and will run the tests on my own machine as well, and will of course have a look at Nicolas' patches

From Nicolas' comment on #12879, I understood that he accepts to make the TestSuite temporarily silent and leave a proper fix to the number theorists. I guess that's a healthy attitude...

I hope that Nicolas also likes the fix for infinite polynomial rings.

Apply trac_12876_category-fix_abstract_class-sk-rel11521.patch trac_12876-reviewer.patch trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch trac_12876_fix_infinite_polynomial_ring.patch

comment:68 follow-up: Changed 7 years ago by SimonKing

Some questions, Nicolas:

The first patch contains

sage: category = FiniteDimensionalModulesWithBasis(QQ) 
sage: X = CombinatorialFreeModule(QQ, [1,2,3], category = category);   X.rename("X") 
sage: Y = CombinatorialFreeModule(QQ, [1,2,3,4], category = category); Y.rename("Y") 
sage: H = Hom(X, Y) 
sage: H.zero().category_for() 
Category of finite dimensional modules with basis over Rational Field

Why are X and Y renamed? There is yet another location where things are renamed, namely

Diagonal functions can be constructed using the ``diagonal`` option:: 

    sage: X = CombinatorialFreeModule(QQ, [1,2,3,4]); X.rename("X") 
    sage: Y = CombinatorialFreeModule(QQ, [1,2,3,4], key="Y"); Y.rename("Y") 
    sage: H = Hom(X, Y)

Here, I also wonder about the "key" argument.

In sage.rings.ring.pyx, you state:

    """
    This temporary alias is here for those instances of :class:`Ring`
    that are not yet properly in the :class:`Rings`() category.

    TESTS::

        sage: A = Ring(ZZ)
        sage: A._Hom_.__module__
        'sage.categories.rings'
        sage: Hom(A,A).__class__
        <class 'sage.rings.homset.RingHomset_generic_with_category'>
    """
    _Hom_ = Rings.ParentMethods.__dict__['_Hom_']

Are there any rings that do not initialise the category, after #9138? Hence, what would happen without that alias? Perhaps I'll test it, after finishing the tests for the original patches.

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

comment:69 Changed 7 years ago by SimonKing

All tests passed, and the patches look good to me, modulo the questions that I asked above. I will now rerun the tests without the alias for _Hom_.

comment:70 Changed 7 years ago by SimonKing

When removing the _Hom_ alias, I get

        sage -t  -force_lib devel/sage/sage/rings/multi_power_series_ring_element.py # 5 doctests failed

So, apparently it is needed.

Changed 7 years ago by SimonKing

Another reviewer patch

comment:71 follow-up: Changed 7 years ago by SimonKing

  • Description modified (diff)
  • Work issues changed from fix doctests and segfaults to add commit message to one patch

I give it a positive review, modulo commit message of trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch and modulo the small reviewer patch that I have attached. I think it should be .. todo:: and not ..TODO:: (note the blank space).

So, when you added a commit message to your patch, then please change it into a positive review!

Apply trac_12876_category-fix_abstract_class-sk-rel11521.patch trac_12876-reviewer.patch trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch trac_12876_fix_infinite_polynomial_ring.patch trac_12876_another_reviewer.patch

comment:72 follow-ups: Changed 7 years ago by SimonKing

Here is the problem with removing the nasty alias:

sage: A.<a,b> = PowerSeriesRing(QQ)
sage: A._is_category_initialized()
False
sage: isinstance(A,Ring)
True

Hence, apparently Ring.__init__ is not called on PowerSeriesRing. That's nasty, but I think in order to make any progress it would be better to leave that to a different ticket.

comment:73 in reply to: ↑ 72 Changed 7 years ago by SimonKing

Replying to SimonKing:

Hence, apparently Ring.__init__ is not called on PowerSeriesRing. That's nasty, but I think in order to make any progress it would be better to leave that to a different ticket.

See #13412

comment:74 in reply to: ↑ 68 Changed 7 years ago by nthiery

Hi Simon,

Sorry, I am slow answering; I was immersed all afternoon in tracking down a nasty bug where UniqueRepresentation? was violated. It turned out that the UniqueRepresentation? dict had to equal entries in its keys (gasp), and that was due to a tricky mro order for a non trivial inheritance involving Category_singleton and FastHash? which got the hash to change over time ... Anyway ...

Replying to SimonKing:

Diagonal functions can be constructed using the ``diagonal`` option:: 

    sage: X = CombinatorialFreeModule(QQ, [1,2,3,4]); X.rename("X") 
    sage: Y = CombinatorialFreeModule(QQ, [1,2,3,4], key="Y"); Y.rename("Y") 
    sage: H = Hom(X, Y)

Why are X and Y renamed? There is yet another location where things are renamed, namely Here, I also wonder about the "key" argument.

I do this fairly systematically in the tests in modules_with_basis. It makes it immediate to check that the domain and codomain are set properly when looking at the repr of a morphism or homset. They key is to force the domain and codomain to be distinct (otherwise, and this happened to me, if the code accidently interchanges the two it will get unnoticed).

Well, and for good or bad, once you rename a given parent in a doctest, you have to rename consistently this parent in all other doctests in the same file. Or to call X.rename() at the end of the doctest.

comment:75 in reply to: ↑ 72 Changed 7 years ago by nthiery

Replying to SimonKing:

Hence, apparently Ring.__init__ is not called on PowerSeriesRing. That's nasty, but I think in order to make any progress it would be better to leave that to a different ticket.

+1

Can you just add a mini comment on #13412 stating that once it's done the alias can be removed?

Thanks!

comment:76 in reply to: ↑ 71 ; follow-up: Changed 7 years ago by nthiery

Replying to SimonKing:

I give it a positive review, modulo commit message of trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch and modulo the small reviewer patch that I have attached. I think it should be .. todo:: and not ..TODO:: (note the blank space).

Thanks for fixing the space. Apparently, there is some sort of consensus for using .. TODO:: rather than .. todo:: (same thing for seealso, ...).

I have checked the change on infinite polynomial rings, and am ok with it.

Do you mind folding all patches together? Now that we have checked the little steps, that will leave a better overview for the future. And that will take care of the commit messages :-)

(or I can do it, but I need to get the latest beta version first).

And then, it's ready for positive review! Yippee!

Cheers,

Nicolas

comment:77 in reply to: ↑ 76 ; follow-up: Changed 7 years ago by SimonKing

Replying to nthiery:

Replying to SimonKing:

I give it a positive review, modulo commit message of trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch and modulo the small reviewer patch that I have attached. I think it should be .. todo:: and not ..TODO:: (note the blank space).

Thanks for fixing the space. Apparently, there is some sort of consensus for using .. TODO:: rather than .. todo:: (same thing for seealso, ...).

I wouldn't mind to have it upper case.

(or I can do it, but I need to get the latest beta version first).

I don't think you need any beta version if you want to add a commit message. I think editing the patch and replacing the line [mq]: trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch by, say #12876: Use generic code for sage.categories.rings.Rings.HomCategory would be enough.

comment:78 in reply to: ↑ 77 Changed 7 years ago by nthiery

Replying to SimonKing:

I don't think you need any beta version if you want to add a commit message. I think editing the patch and replacing the line [mq]: trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch by, say #12876: Use generic code for sage.categories.rings.Rings.HomCategory would be enough.

I meant for folding the patches together!

comment:79 follow-up: Changed 7 years ago by SimonKing

Why folding? Perhaps that's a question to the release manager. But are four patches substantially more difficult to merge than one patch?

comment:80 in reply to: ↑ 79 Changed 7 years ago by nthiery

Replying to SimonKing:

Why folding? Perhaps that's a question to the release manager. But are four patches substantially more difficult to merge than one patch?

It's not so much about the release manager than for whoever will come back to this ticket in the future, and will want to have a synthetic view of what the patch(es) does.

Separate patches are temporarily good for incremental review. They can be interesting too in the long run when they do independent changes. But when they pile on top of each other (typically for trivial typo fixes) it goes in the way of the reader. Typically (s)he will read the main patch, see a typo, wonder if it has been fixed, will have to dig through the followup patches, think about the order in which they are to be applied, etc.

comment:81 follow-up: Changed 7 years ago by SimonKing

  • Dependencies changed from #11521, #12215, #12313 to #11521, #12215, #12313, #13412
  • Description modified (diff)

I produced a combined patch. I added #13412, because it allows to remove the ugly alias for _Hom_ in sage.rings.ring. Because of that last change, I keep it "needs review" for now, but will change it into positive review provided the tests pass.

Apply trac_12876_category_abstract_classes_for_hom.patch

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

comment:82 in reply to: ↑ 81 Changed 7 years ago by nthiery

Replying to SimonKing:

I produced a combined patch. I added #13412, because it allows to remove the ugly alias for _Hom_ in sage.rings.ring. Because of that last change, I keep it "needs review" for now, but will change it into positive review provided the tests pass.

Apply trac_12876_category_abstract_classes_for_hom.patch

Excellent, thanks!

Is there any of the old patches that you would like to keep for posterity, or should I just wipe them up?

Cheers,

comment:83 Changed 7 years ago by SimonKing

I wouldn't remove the old patches from trac - and actually I wouldn't be able to remove them -, unless there are problems with disc space etc. And IF there will eventually be problems with disc space, I guess a better solution would be to start deleting the attachments of tickets that are merged since, say, at least one year.

comment:84 Changed 7 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues changed from add commit message to one patch to Understand why the crash in infinite_polynomial_ring is not fixed and what happens with r

That's bad. Pathbot reports

	sage -t  -force_lib devel/sage-12876/sage/rings/polynomial/infinite_polynomial_ring.py # Killed/crashed
	sage -t  -force_lib devel/sage-12876/sage/interfaces/r.py # 1 doctests failed

The crash in infinite_polynomial_ring.py was supposed to be fixed with one of the folded patches, namely removing the additional weak cache. The second one has occurred on my computer as well - but was not reproducible, so I thought it was just some noise.

Bad.

comment:85 Changed 7 years ago by SimonKing

I tried to stress-test my machine, running 16 copies of the infinite_polynomial_ring tests in parallel on only 4 kernels, but I was not able to reproduce the segfault.

Concerning the other error, perhaps one can add the following patch:

  • sage/interfaces/r.py

    diff --git a/sage/interfaces/r.py b/sage/interfaces/r.py
    a b  
    11661166
    11671167        ::
    11681168
    1169             sage: os.path.realpath(tmpdir) == sageobj(r.getwd())
     1169            sage: os.path.realpath(tmpdir) == os.path.realpath(sageobj(r.getwd()))
    11701170            True
    11711171        """
    11721172        self.execute('setwd(%r)' % dir)

comment:86 Changed 7 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Let's try, even though I have no idea about the segfault:

Apply trac_12876_category_abstract_classes_for_hom.patch trac_12876_r_test.patch

comment:87 Changed 7 years ago by SimonKing

OK, the r-test works now. But the segfault remains. I wish I could reproduce it, preferably in verbose mode.

comment:88 Changed 7 years ago by SimonKing

I tried to valgrind the failing segfault (Nils Bruin recommended valgrind and posted a sage.supp at #11918), in the hope that it'll find something fishy even if it doesn't result in a segfault.

When I run the tests of sage/rings/polynomial/infinite_polynomial_ring.py, I do not get a SIGILL. But I do get a considerable amount of lost memory:

==13541== 13,936 bytes in 1 blocks are definitely lost in loss record 8,673 of 8,997
==13541==    at 0x4C244E8: malloc (vg_replace_malloc.c:236)
==13541==    by 0x21E8984F: omAllocFromSystem (omAllocSystem.c:184)
==13541==    by 0x21E89A21: omAllocLarge (omAllocSystem.c:39)
==13541==    by 0x21BB3A00: iiAllStart(procinfo*, char*, feBufferTypes, int) (omalloc.h:2432)
==13541==    by 0x21BB3B95: iiPStart(idrec*, sleftv*) (iplib.cc:360)
==13541==    by 0x21BB4148: iiMake_proc(idrec*, sip_package*, sleftv*) (iplib.cc:482)
==13541==    by 0x2239B64D: __pyx_f_4sage_4libs_8singular_8function_call_function(__pyx_obj_4sage_4libs_8singular_8function_SingularFunction*, _object*, _object*, __pyx_opt_args_4sage_4libs_8singular_8f
unction_call_function*) (function.cpp:13241)
==13541==    by 0x2239CBA8: __pyx_pw_4sage_4libs_8singular_8function_16SingularFunction_5__call__(_object*, _object*, _object*) (function.cpp:11924)
==13541==    by 0x4E742C2: PyObject_Call (abstract.c:2529)
==13541==    by 0x4F160FC: PyEval_EvalFrameEx (ceval.c:4239)
==13541==    by 0x4F19124: PyEval_EvalCodeEx (ceval.c:3253)
==13541==    by 0x4E9C122: function_call (funcobject.c:526)
==13541==    by 0x4E742C2: PyObject_Call (abstract.c:2529)
==13541==    by 0x4F14C59: PyEval_EvalFrameEx (ceval.c:4334)
==13541==    by 0x4F19124: PyEval_EvalCodeEx (ceval.c:3253)
==13541==    by 0x4E9C122: function_call (funcobject.c:526)
==13541==    by 0x4E742C2: PyObject_Call (abstract.c:2529)
==13541==    by 0x4F14C59: PyEval_EvalFrameEx (ceval.c:4334)
==13541==    by 0x4F19124: PyEval_EvalCodeEx (ceval.c:3253)
==13541==    by 0x4E9C122: function_call (funcobject.c:526)
==13541==    by 0x4E742C2: PyObject_Call (abstract.c:2529)
==13541==    by 0xB29B841: __pyx_pw_4sage_4misc_9cachefunc_12CachedMethod_3_instance_call (cachefunc.c:9733)
==13541==    by 0x4E742C2: PyObject_Call (abstract.c:2529)
==13541==    by 0xB29C7D4: __pyx_pw_4sage_4misc_9cachefunc_18CachedMethodCaller_7__call__ (cachefunc.c:7254)
==13541==    by 0x4E742C2: PyObject_Call (abstract.c:2529)

Is there a way to find out what singular_function or what cached method are involved?

comment:89 Changed 7 years ago by SimonKing

  • Dependencies changed from #11521, #12215, #12313, #13412 to #11521, #12215, #12313, #13412. #13145

It seems that #13145 fixes the segfault that was sometimes observed at #13370. Perhaps it helps here as well? Let's try!

comment:90 Changed 7 years ago by SimonKing

Too bad, #13145 didn't help.

I'd really need some help to trace the problem down. Could someone run the test in verbose mode on one of the machines exposing the segfault, so that one can see whether the problem is during the tests or while shutting down Sage? Can someone produce (and post) a backtrace?

comment:91 Changed 7 years ago by SimonKing

There is a potential name conflict between an attribute S of Action and RingHomomorphism. The former has been turned into a weak reference at #715. I am now trying to rename the attribute of Action, and updated the main patch at #715 accordingly.

Hence, kicking the patchbot now. Hope it works!

comment:92 Changed 7 years ago by SimonKing

It doesn't work. Hence, again I'd like to finally see a backtrace of the segfault. So far, I tried to reproduce the segfault on my linux laptop, but to no avail. Next, I'll try bsd.math.

comment:93 Changed 7 years ago by SimonKing

Now I am totally puzzled. On bsd.math, admittedly with the new GAP 4.5.5 spkg, I get

The following tests failed:

        sage -t  -force_lib devel/sage/sage/combinat/combinatorial_algebra.py # 4 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/partition.py # 3 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/classical.py # 11 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/dual.py # 89 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/elementary.py # 9 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/hall_littlewood.py # 61 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/homogeneous.py # 9 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/jack.py # 68 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/kschur.py # 15 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/llt.py # 50 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/monomial.py # 16 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/ns_macdonald.py # 2 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/macdonald.py # 106 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/orthotriang.py # 25 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/powersum.py # 17 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/schur.py # 13 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/sf/sfa.py # 291 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/species/composition_species.py # 4 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/species/functorial_composition_species.py # 3 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/species/generating_series.py # 38 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/species/library.py # 4 doctests failed
        sage -t  -force_lib devel/sage/sage/combinat/species/species.py # 4 doctests failed

but no segfault. Does one need #5457?

comment:94 follow-up: Changed 7 years ago by jdemeyer

Did you forget a ./sage -b by chance?

comment:95 in reply to: ↑ 94 Changed 7 years ago by SimonKing

Replying to jdemeyer:

Did you forget a ./sage -b by chance?

Doesn't make ptest start with ./sage -b?

comment:96 Changed 7 years ago by SimonKing

It is really really REALLY frustrating. Volker's patchbot keeps reporting a segfault, but I can not reproduce it on bsd.math, With #5457 and the new GAP spkg, make ptest just reports a single error:

sage -t -force_lib "devel/sage/sage/interfaces/gap.py"      
**********************************************************************
File "/scratch/sking/sage-5.3.rc1/devel/sage/sage/interfaces/gap.py", line 809:
    sage: gap(2)
Expected:
    2
Got:
    <BLANKLINE>
**********************************************************************
1 items had failures:
   1 of   4 in __main__.example_21
***Test Failed*** 1 failures.
For whitespace errors, see the file /Users/SimonKing/.sage//tmp/gap_70056.py
         [29.6 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t -force_lib "devel/sage/sage/interfaces/gap.py"
Total time for all tests: 29.7 seconds

That's rather strange, but I guess that's the problem of the GAP spkg.

In any case, I don't see a segfault. Not on my laptop, not on the machine in my office, not on bsd.math.

comment:97 Changed 7 years ago by SimonKing

Meanwhile #11521 has a new dependency, namely #13447, that is supposed to fix the segfaults (which apparently came from a wrong refcount in libsingular).

I am now running make ptest on bsd.math.washington.edu, with

$ hg qa
trac_715_combined.patch
trac_715_local_refcache.patch
trac_715_safer.patch
trac_715_specification.patch
trac_11521_homset_weakcache_combined.patch
trac_11521_callback.patch
13145.patch
trac_13447-sanitise_ring_refcount.patch
trac12215_weak_cached_function-sk.patch
trac12215_segfault_fixes.patch
trac_12313-mono_dict-combined-random-sk.patch
trac_12313_quit_sage.patch
trac13370_deprecate_is_field.patch
trac_13378-convert_map_shortcut.patch
trac_13412_category_for_power_series_rings.patch
trac_12876_category_abstract_classes_for_hom.patch
trac_12876_r_test.patch

applied on top of sage-5.4.beta0.

comment:98 Changed 7 years ago by SimonKing

Hooray, all tests pass :)

comment:99 Changed 7 years ago by nthiery

Nice!

Anything left to be done to complete the review?

comment:100 Changed 7 years ago by SimonKing

  • Status changed from needs_review to positive_review
  • Work issues Understand why the crash in infinite_polynomial_ring is not fixed and what happens with r deleted

The work issue was "understand the crash of r". I think it is now understood, because it could be solved by using os.realpath in some test, where apparently a relative path was stored.

All segfaults that I have seen are gone because of #13447. But it would be nice if Volker's patchbot could try it again, because it seems to have a tendency to break in a different way than my machines.

I have read the patch carefully in the past, and all tests pass, with the patches stated in comment:97.

Hence: Positive review, I'd say.

comment:101 Changed 7 years ago by jdemeyer

  • Dependencies changed from #11521, #12215, #12313, #13412. #13145 to #715, #11521, #12215, #12313, #13412. #13145

comment:102 Changed 7 years ago by jdemeyer

  • Dependencies changed from #715, #11521, #12215, #12313, #13412. #13145 to #13447, #715, #11521, #12215, #12313, #13412. #13145
  • Milestone changed from sage-5.4 to sage-pending

comment:103 Changed 7 years ago by nthiery

  • Dependencies changed from #13447, #715, #11521, #12215, #12313, #13412. #13145 to #715, #11521, #12215, #12313, #13412. #13145
  • Status changed from positive_review to needs_work
  • Work issues set to needs rebase

comment:104 Changed 7 years ago by nthiery

As per the discussion on #13347, I have removed this dependency.

The patch needs to be rebased upon #14159 and #13184 that have been merged in 5.10beta0. I am working on it.

comment:105 Changed 7 years ago by nthiery

I just uploaded a rebased patch. This patch sounds almost good to go.

Still, there are a couple failing tests which all boil to the following little design issue:

Take as typical situation A to be Rings().parent_class, and B to be some NumberField? parent class. Assume that P is an instance of A resp. B and that:

Let P be an instance of B and codomain a ring. With the current logic Hom(P, codomain) will try the later, which will fail, but won't proceed to try the former. So the result will be a plain Homset, and not a RingHomset? as desired.

For some reasons this situation did not appear a year ago. Presumably some tests were added in the mean time. Or we did not run the tests with --long ...

What's the best way out?

  • Option 1: Walk the mro of the class of P, and call cls._Hom_(P, codomain) in turn until something does not fail
  • Option 2: change the protocol so that the _Hom_ method should call super instead of failing if they cannot handle the situation
  • Option 3: ???

Cheers,

Nicolas

comment:106 Changed 7 years ago by nthiery

  • Work issues changed from needs rebase to failing tests; design issue

comment:107 Changed 7 years ago by nthiery

  • Status changed from needs_work to needs_review
  • Work issues failing tests; design issue deleted

Two failures were actually trivial:

  • A trivial doctest in devel/sage/doc/en/thematic_tutorials/coercion_and_categories.rst
  • More skips were needed in the new TestSuite?(H) in line 114 of sage.categories.hecke_modules (some new test methods that require the currently missing an_element were added since last year)

I implemented a simplistic protocol for the above issue. If P._Hom_ fails, then try category.parent_class._Hom_. This is sufficient for the other failing test in Parent.

Other than this, the attached patch is a straightforward rebase of the previous one. So to finalize the review it would suffice to:

  • glance at the above change
  • recheck sage.categories.homset.Hom

I am currently running the long tests, but I expect everything to pass.

Almost done!

comment:108 Changed 7 years ago by nthiery

make ptestlong passed smoothly on my machine!

comment:109 Changed 6 years ago by SimonKing

Are there dependencies missing? With sage-5.9.rc0, which should contain all the dependencies listed, I get

> hg qimport -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/12876/trac_12876_category_abstract_classes_for_hom.patch
Füge trac_12876_category_abstract_classes_for_hom.patch zur Seriendatei hinzu
Wende trac_12876_category_abstract_classes_for_hom.patch an
Wende Patch auf Datei doc/en/thematic_tutorials/coercion_and_categories.rst an
FEHLSCHLAG von Teilstück #1 in Zeile 407
1 von 1 Teilstücken sind FEHLGESCHLAGEN -- speichere Ausschuss in Datei doc/en/thematic_tutorials/coercion_and_categories.rst.rej
Wende Patch auf Datei sage/categories/homset.py an
Teilstück #1 wurde erfolgreich in Zeile 69 mit Unschärfe 2 angewandt (0 Zeilen verschoben).
FEHLSCHLAG von Teilstück #4 in Zeile 181
FEHLSCHLAG von Teilstück #7 in Zeile 273
2 von 12 Teilstücken sind FEHLGESCHLAGEN -- speichere Ausschuss in Datei sage/categories/homset.py.rej
Wende Patch auf Datei sage/modules/vector_space_homspace.py an
FEHLSCHLAG von Teilstück #1 in Zeile 327
1 von 1 Teilstücken sind FEHLGESCHLAGEN -- speichere Ausschuss in Datei sage/modules/vector_space_homspace.py.rej
Patch schlug fehl und Fortsetzung unmöglich (versuche -v)
Patch schlug fehl, Fehlerabschnitte noch im Arbeitsverzeichnis
Fehler beim Anwenden. Bitte beheben und trac_12876_category_abstract_classes_for_hom.patch aktualisieren

Hence, several failures in applying the first patch.

It seems that the patchbot applies the patches in the wrong order anyway (and also gets a failure).

To let the patchbot get it right:

Apply trac_12876_category_abstract_classes_for_hom.patch trac_12876_r_test.patch

comment:110 Changed 6 years ago by nthiery

  • Dependencies changed from #715, #11521, #12215, #12313, #13412. #13145 to #715, #11521, #12215, #12313, #13412, #13145, #14159, #13184

Yes; I rebased the patch upon #14159 and #13184 which have been merged in 5.10 beta0. I just added them to the dependency list for the record.

comment:111 Changed 6 years ago by SimonKing

  • Dependencies changed from #715, #11521, #12215, #12313, #13412, #13145, #14159, #13184 to #715, #11521, #12215, #12313, #13412, #13145, #14159, #13184, #14287

#14287 is also needed. With this, the patch applies.

comment:112 Changed 6 years ago by SimonKing

However, there seems to be yet another dependency for the second patch.

comment:113 Changed 6 years ago by SimonKing

It fails to apply, because in r.py it reads

           sage: os.path.realpath(tmpdir) == sageobj(r.getwd())  # known bug (:trac:`9970`)

but the second patch from here assumes that there is no pointer to #9970.

comment:114 Changed 6 years ago by SimonKing

The pointer got introduced in #12415, as it seems.

comment:115 Changed 6 years ago by SimonKing

After applying the failing hunk manually, I am now running make ptestlong.

comment:116 follow-up: Changed 6 years ago by kcrisman

I don't think that the reference to #9970 should be removed. It's okay to put in a different test for now if that helps things, but it doesn't remove the underlying problem in our parsing that wjp has a tentative patch for there, so we shouldn't just completely not mention it. See this comment by Simon for background.

comment:117 in reply to: ↑ 116 Changed 6 years ago by SimonKing

Replying to kcrisman:

I don't think that the reference to #9970 should be removed. It's okay to put in a different test for now if that helps things, but it doesn't remove the underlying problem in our parsing that wjp has a tentative patch for there, so we shouldn't just completely not mention it. See this comment by Simon for background.

In any case, the second patch needs to be rebased against #12415, right?

comment:118 follow-up: Changed 6 years ago by nthiery

Simon: are you sure we need trac_12876_r_test.patch? It's a patch that you had added and actually I forgot to insert it in the sage-combinat queue; and all tests are passing smoothly for me without it!

comment:119 in reply to: ↑ 118 Changed 6 years ago by SimonKing

  • Description modified (diff)

Replying to nthiery:

Simon: are you sure we need trac_12876_r_test.patch? It's a patch that you had added and actually I forgot to insert it in the sage-combinat queue; and all tests are passing smoothly for me without it!

Really?

OK, then let's try:

Apply trac_12876_category_abstract_classes_for_hom.patch

comment:120 follow-up: Changed 6 years ago by kcrisman

It's a patch that you had added and actually I forgot to insert it in the sage-combinat queue; and all tests are passing smoothly for me without it!

This is a pretty unstable situation - it doesn't appear all the time and not on all platforms. I think that the idea of replacing this with a test that works for now is fine - see Simon's suggestion at #9970.

comment:121 in reply to: ↑ 120 Changed 6 years ago by nthiery

Replying to kcrisman:

This is a pretty unstable situation - it doesn't appear all the time and not on all platforms. I think that the idea of replacing this with a test that works for now is fine - see Simon's suggestion at #9970.

I am fine with that. I just still need to understand at some point how this is related to this ticket :-)

Changed 6 years ago by nthiery

comment:122 Changed 6 years ago by nthiery

With the attached extra patch, all test still pass and there is no more crappy new in Schemes.HomCategory?.ParentMethods?.

comment:123 Changed 6 years ago by nthiery

  • Description modified (diff)

comment:124 Changed 6 years ago by SimonKing

Patchbot times out at sage/interfaces/ecm.py. However, with the first two patches, make ptestlong passes.

My plan is to modify the second patch, so that reference to #9970 is preserved, and to repeat make ptestlong with the third patch.

comment:125 Changed 6 years ago by SimonKing

What other dependency is there for the third patch?

comment:126 follow-up: Changed 6 years ago by SimonKing

I have

> hg qa
trac_14159_weak_value_triple_dict.patch
trac_14159_use_cdef_get.patch
trac_13184_sage_5.9.beta.patch
trac_14287-rebased.patch
trac_12876_category_abstract_classes_for_hom.patch
trac_12876_r_test.patch

on top of sage-5.9.rc0, and the relevant lines in sage/categories/schemes.py are

            """
            return []

        class ParentMethods:

            def __new__(cls, R, S, category):
                """
                TESTS::

                    sage: E = EllipticCurve('37a1')
                    sage: Hom(E, E).__class__
                    <class 'sage.schemes.generic.homset.SchemeHomset_generic_with_category'>

                If both schemes R and S are actually specs, we want
                the parent for Hom(R, S) to be in a different class::

                    sage: Hom(Spec(ZZ), Spec(ZZ)).__class__
                    <class 'sage.schemes.generic.homset.SchemeHomset_points_spec_with_category'>

                Currently, and to minimize the changes, this is done
                by delegating the job to SchemeHomset. This is not
                very robust: for example, only one category can do
                this hack.

                FIXME: this might be better handled by an extra Spec category
                """
                from sage.schemes.generic.homset import SchemeHomset
                return SchemeHomset(R, S, category=category)


#############################################################

However, the patch expects

             """
             return []
 
-        class ParentMethods:
-
-            def __new__(cls, R, S, category):
-                """
-                TESTS::
-
-                    sage: E = EllipticCurve('37a1')
-                    sage: Hom(E, E).__class__
-                    <class 'sage.schemes.generic.homset.SchemeHomset_generic_with_category'>
-
-                If both schemes R and S are actually specs, we want
-                the parent for Hom(R, S) to be in a different class::
-
-                    sage: Hom(Spec(ZZ), Spec(ZZ)).__class__
-                    <class 'sage.schemes.affine.affine_homset.SchemeHomset_points_spec_with_category'>
-
-                Currently, and to minimize the changes, this is done
-                by delegating the job to SchemeHomset. This is not
-                very robust: for example, only one category can do
-                this hack.
-
-                FIXME: this might be better handled by an extra Spec category
-                """
-                from sage.schemes.generic.homset import SchemeHomset
-                return SchemeHomset(R, S, category=category)
 
 
 #############################################################

So, what tickect changed sage.schemes.generic.homset.SchemeHomset_points_spec into sage.schemes.affine.affine_homset.SchemeHomset_points_spec?

comment:127 Changed 6 years ago by SimonKing

  • Dependencies changed from #715, #11521, #12215, #12313, #13412, #13145, #14159, #13184, #14287 to #715, #11521, #12215, #12313, #13412, #13145, #14159, #13184, #14287, #14217

It is #14217.

Changed 6 years ago by SimonKing

Try to make one test in interfaces/r.py more stable wrt symbolic links

comment:128 Changed 6 years ago by SimonKing

  • Description modified (diff)

Patchbot: Apply trac_12876_category_abstract_classes_for_hom.patch trac_12876_r_test.py trac_12876-schemes_homcategory.patch

comment:129 Changed 6 years ago by jdemeyer

I really don't like trac_12876_r_test.patch. Are we sure the test actually works now (you removed # known bug)?

comment:130 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:131 Changed 6 years ago by jdemeyer

Given that the other patches don't change anything in the R interface, it is almost impossible that this ticket could have any influence on the R test. Or at least, it requires a good explanation if something did change.

Proposal: do not apply trac_12876_r_test.patch

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:132 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_info

comment:133 follow-up: Changed 6 years ago by SimonKing

I'm fine with not applying the r-test patch. An info that I need: Does that tag # known bug mean that the patchbot will not complain about it?

comment:134 Changed 6 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_info to needs_review

Patchbot:

Apply trac_12876_category_abstract_classes_for_hom.patch trac_12876-schemes_homcategory.patch

comment:135 in reply to: ↑ 133 Changed 6 years ago by jdemeyer

Replying to SimonKing:

Does that tag # known bug mean that the patchbot will not complain about it?

Yes, see http://sagemath.org/doc/developer/conventions.html#further-conventions-for-automated-testing-of-examples

comment:136 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.10

comment:137 in reply to: ↑ 126 Changed 6 years ago by nthiery

on top of sage-5.9.rc0, and the relevant lines in sage/categories/schemes.py are

Time for an upgrade maybe? :-)

Version 0, edited 6 years ago by nthiery (next)

comment:138 follow-up: Changed 6 years ago by nthiery

Patchbot is green :-)

comment:139 in reply to: ↑ 138 ; follow-up: Changed 6 years ago by SimonKing

Replying to nthiery:

Patchbot is green :-)

Yes, I know. I need some more time to finish yet another job application.

comment:140 in reply to: ↑ 139 Changed 6 years ago by nthiery

Replying to SimonKing:

Yes, I know. I need some more time to finish yet another job application.

I did not mean to pressure; I was just happy :-)

Of course job applications have priority! Good luck!

comment:141 follow-up: Changed 6 years ago by SimonKing

From the commit message:

  • If category is None, Hom simply calls itself with the meet of the categories of the parent, which removes a cache handling duplication.

I don't know if that's a good idea. Duplicating the cache costs virtually nothing (because all objects, categories and homsets involved are there anyway), but it speeds up the most commonly used case, namely category=None. Computing the meet of the underlying categories is expensive, even though it is cached.

comment:142 follow-up: Changed 6 years ago by SimonKing

Question: Is it really needed that each category defines its own HomCategory, which in most cases is just

    class HomCategory(HomCategory):
        pass

?

Since all classes for categories inherit from Category, couldn't one just define the HomCategory there, and only replace it when really needed?

This could be for a different ticket, though. I just wonder.

Last edited 6 years ago by SimonKing (previous) (diff)

comment:143 follow-up: Changed 6 years ago by SimonKing

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.

comment:144 Changed 6 years ago by SimonKing

  • Status changed from needs_review to needs_info

Tests passed. I went through both patches, and they look mostly good, except for the minor criticism in comment:141 and comment:142. It is almost a positive review, but perhaps you have a comment on these two points.

comment:145 in reply to: ↑ 141 Changed 6 years ago by nthiery

Replying to SimonKing:

From the commit message:

  • If category is None, Hom simply calls itself with the meet of the categories of the parent, which removes a cache handling duplication.

I don't know if that's a good idea. Duplicating the cache costs virtually nothing (because all objects, categories and homsets involved are there anyway), but it speeds up the most commonly used case, namely category=None. Computing the meet of the underlying categories is expensive, even though it is cached.

Oh you are right. I meant to avoid a duplication in the source code, not in the caching itself. I agree with you, the current code is wrong. I am going to fix this right now.

Thanks for catching this!

Changed 6 years ago by nthiery

comment:146 Changed 6 years ago by nthiery

  • Description modified (diff)
  • Status changed from needs_info to needs_review

The new uploaded patch is the previous one with

folded in. The latter should fix the caching of Hom(X,Y).

I am currently running all long tests!

comment:147 in reply to: ↑ 142 Changed 6 years ago by nthiery

Replying to SimonKing:

Question: Is it really needed that each category defines its own HomCategory, which in most cases is just

    class HomCategory(HomCategory):
        pass

?

Since all classes for categories inherit from Category, couldn't one just define the HomCategory there, and only replace it when really needed?

Well, most categories actually don't define a HomCategory?. I did not strip the now trivial HomCategory? in Schemes just as to avoid having to fix some trivial doctest failures elsewhere (without it, a Hom(scheme,scheme) was downgraded to the category of homsets of sets).

This could be for a different ticket, though. I just wonder.

Yup. At this point we have very little use cases of HomCategories?, and I don't have a good view on how often this gadget will be used, and thus what the sane default should be. So I would indeed leave things untouched for now until we have more experience.

comment:148 follow-up: Changed 6 years ago by SimonKing

You should also tell the patchbot what to do.

Apply trac_12876_category_abstract_classes_for_hom.patch

comment:149 in reply to: ↑ 143 Changed 6 years ago by nthiery

Replying to SimonKing:

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 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 :-)

comment:150 in reply to: ↑ 148 Changed 6 years ago by nthiery

Replying to SimonKing:

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.

comment:151 Changed 6 years ago by nthiery

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!

comment:152 Changed 6 years ago by nthiery

It seems to be happy :-)

comment:153 Changed 6 years ago by SimonKing

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.

Changed 6 years ago by SimonKing

comment:154 Changed 6 years ago by SimonKing

  • Description modified (diff)

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

comment:155 follow-up: Changed 6 years ago by nbruin

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

comment:156 in reply to: ↑ 155 Changed 6 years ago by nthiery

Replying to nbruin:

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

Yup. I am on it.

Changed 6 years ago by nthiery

Combined patch

comment:157 Changed 6 years ago by nthiery

  • Description modified (diff)

comment:158 Changed 6 years ago by nthiery

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

Apply:

comment:159 Changed 6 years ago by SimonKing

  • Status changed from needs_review to positive_review

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

comment:160 Changed 6 years ago by SimonKing

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

Apply trac_12876_category_abstract_classes_for_hom.patch

comment:161 Changed 6 years ago by nthiery

Youpi!

Thanks Simon!

comment:162 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:163 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.11.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.