Opened 8 years ago
Last modified 7 years ago
#12876 closed enhancement
Fix element and parent classes of Hom categories to be abstract, and simplify the Hom logic. — at Version 86
Reported by:  nthiery  Owned by:  nthiery 

Priority:  major  Milestone:  sage5.11 
Component:  categories  Keywords:  categories, Hom 
Cc:  sagecombinat, SimonKing  Merged in:  
Authors:  Nicolas M. Thiéry  Reviewers:  Simon King 
Report Upstream:  N/A  Work issues:  Understand why the crash in infinite_polynomial_ring is not fixed and what happens with r 
Branch:  Commit:  
Dependencies:  #11521, #12215, #12313, #13412  Stopgaps: 
Description (last modified by )
This patch fixes the parent and element classes for Hom categories to be purely abstract, and simplifies the Hom logic:
 Unified the logic for selecting the class when building a Homset (e.g. Homset, RingHomset?, HeckeModuleHomspace?, ...). This is now systematically done through the _Hom_ hook. The logic still has a fundamental flaw, but that's for the later #10668.
 The cache for Hom is handled at a single point in Hom In particular, homsets created via the _Hom_ hook are now unique.
 If category is None, Hom simply calls itself with the meet of the categories of the parent, which removes a cache handling duplication.
 Parent.Hom calls Hom directly (removes duplicate _Hom_ logic).
 ParentWithBase?.Hom was redundant and is gone.
 Reduce the footprint of the current trick to delegate Hom(F,F)(on_basis=...) to module_morphism, allow for the diagonal option too, an make sure the homset category is set properly.
 Update a doctest in sage.modules.vector_space_homspace to take into account that homsets created via _Hom_ are now unique.
 Scheme is (apparently) an abstract base class; so it should not be instantiated. I changed some doctests in sage.schemes.generic.SchemeMorphism? to use instead the concrete Spec(ZZ). Those doctests were breaking because Scheme does not implement equality, which is required for Hom caching.
As a byproduct, the HeckeModules? category does not import any more HeckeModulesHomspace?, which was a recurrent source of import loops.
#11935 depends on this ticket
Apply:
Change History (92)
comment:1 Changed 8 years ago by
 Status changed from new to needs_review
comment:2 Changed 8 years ago by
 Description modified (diff)
comment:3 Changed 8 years ago by
 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/sagecombinat/sage/rings/polynomial/multi_polynomial_libsingular.pyx ********************************************************************** File "/opt/sage5.0.beta11/devel/sagecombinat/sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 418: sage: len(ring_refcount_dict) == n Expected: True Got: False sage t devel/sagecombinat/sage/libs/singular/ring.pyx ********************************************************************** File "/opt/sage5.0.beta11/devel/sagecombinat/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
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
 Description modified (diff)
comment:6 Changed 8 years ago by
Oops. The updated patch includes two further hunks I had forgotten.
comment:7 Changed 8 years ago by
Arr, yet another missing hunk ... Time to go to bed!
comment:8 followup: ↓ 10 Changed 8 years ago by
 Dependencies set to #12877
comment:9 Changed 8 years ago by
 Cc SimonKing added
comment:10 in reply to: ↑ 8 Changed 8 years ago by
 Dependencies changed from #12877 to #12875, #12877
comment:11 followup: ↓ 13 Changed 8 years ago by
 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 sage5.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/sage5.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/sage5.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
PS: I just verified that the problem also occurs without #12808.
comment:13 in reply to: ↑ 11 Changed 8 years ago by
Replying to SimonKing:
With #12808, #12875 and #12877 applied on top of sage5.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
comment:15 Changed 8 years ago by
comment:16 Changed 8 years ago by
PS: I am just attempting a rebase, to see whether #11521 fixes the ring_refcount_dict problem.
comment:17 followup: ↓ 25 Changed 8 years ago by
Here is the rejection:

homset.py
def Hom(X, Y, category=None): 165 203 if H.domain() is X and H.codomain() is Y: 166 204 return H 167 205 168 try:169 return X._Hom_(Y, category)170 except (AttributeError, TypeError):171 pass172 173 206 cat_X = X.category() 174 207 cat_Y = Y.category() 175 208 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): 183 211 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) 196 216 197 217 # construct H 198 218 # Design question: should the Homset classes get the category or the homset category? 199 219 # For the moment, this is the category, for compatibility with the current implementations 200 220 # 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) 205 227 return H 206 228 207 229 def hom(X, Y, f):
Why do you import Sets (new line 221)?
Do I understand correctly: If Hom receives None as category, then you determine the category as the meet of the categories of domain and codomain, and call Hom again. Did you test that the calling overhead does not matter?
comment:18 followup: ↓ 28 Changed 8 years ago by
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.
comment:19 Changed 8 years ago by
 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_categoryfix_abstract_classntrel11521.patch
comment:20 Changed 8 years ago by
Hm. With sage5.0.beta13 plus #715, #11521, #12875, #12877 and trac_12876_categoryfix_abstract_classntrel11521.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 followup: ↓ 26 Changed 8 years ago by
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 followup: ↓ 27 Changed 8 years ago by
 Dependencies changed from #12875, #12877 to #715, #11521, #12875, #12877, #12215
 Description modified (diff)
With sage5.0.beta13 plus #715, #11521, #12875, #12877, #12215 and trac_12876_categoryfix_abstract_classntrel11521.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_categoryfix_abstract_classntrel11521.patch trac_12876reviewer.patch
Changed 8 years ago by
comment:23 Changed 8 years ago by
Sorry, I made a lastminute 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_12876reviewer.2.patch, which was created by mistake.
Apply trac_12876_categoryfix_abstract_classntrel11521.patch trac_12876reviewer.patch
comment:24 Changed 8 years ago by
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
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
comment:27 in reply to: ↑ 22 Changed 8 years ago by
Hi Simon!
Replying to SimonKing:
With sage5.0.beta13 plus #715, #11521, #12875, #12877, #12215 and trac_12876_categoryfix_abstract_classntrel11521.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
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
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/sagecombinat/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
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 followup: ↓ 32 Changed 8 years ago by
 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/sage5.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/sage5.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/sage5.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/sage5.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 sage5.0.beta13 plus #715, #11521, #12875, #12877, #12215 and #12876.
comment:32 in reply to: ↑ 31 Changed 8 years ago by
Replying to SimonKing:
I have sage5.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/sage5.0.beta13/devel/sage$ hg qa trac_715_combined.patch trac_11521_homset_weakcache_combined.patch trac_12875categoryfix_abvar_homspacent.patch trac_12877categoryfor_more_rings_and_schemesnt.patch trac12215_weak_cached_function.patch trac12215_segfault_fixes.patch trac_12876_categoryfix_abstract_classntrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch
comment:33 Changed 8 years ago by
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
Changed 8 years ago by
comment:34 followup: ↓ 36 Changed 8 years ago by
 Work issues changed from Fix ring_refcount_dict problem to Fix the reviewnt patch
trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch doesn't work, because you confused ` with '.
Please change the following hunk:

hecke_modules.py
class HeckeModules(Category_module): 116 116 Fixing :meth:`_test_zero` (``__call__`` should accept a 117 117 function as input) and :meth:`_test_elements` (modular 118 118 form morphisms elements should inherit from categories) is 119 :trac:` ???`.119 :trac:`12879`. 120 120 121 121 TESTS::
since the tobechanged file has :trac:'???'
, hence, no backticks around ???.
comment:35 followup: ↓ 37 Changed 8 years ago by
 Work issues changed from Fix the reviewnt patch to Fix the reviewnt 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/sage5.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/sage5.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/sage5.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/sage5.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/sage5.0.beta13/devel/sage$ hg qapplied trac_12808classcall_speedupfh.patch trac_12808_nested_class_cython.patch trac_12808classcall_cdef.patch trac12215_weak_cached_function.patch trac12215_segfault_fixes.patch trac_715_combined.patch trac_11521_homset_weakcache_combined.patch trac_12875categoryfix_abvar_homspacent.patch trac_12877categoryfor_more_rings_and_schemesnt.patch trac_12876_categoryfix_abstract_classntrel11521.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch
How is that different from your setting?
comment:36 in reply to: ↑ 34 ; followup: ↓ 38 Changed 8 years ago by
Replying to SimonKing:
trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch doesn't work, because you confused ` with '.
Please change the following hunk:
hecke_modules.py
class HeckeModules(Category_module): 116 116 Fixing :meth:`_test_zero` (``__call__`` should accept a 117 117 function as input) and :meth:`_test_elements` (modular 118 118 form morphisms elements should inherit from categories) is 119 :trac:` ???`.119 :trac:`12879`. 120 120 121 121 TESTS:: since the tobechanged file has
:trac:'???'
, hence, no backticks around ???.
Did you include your trac_12876reviewer.patch (which fixes those backticks) in between trac_12876_categoryfix_abstract_classntrel11521.patch and trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch?
comment:37 in reply to: ↑ 35 ; followup: ↓ 39 Changed 8 years ago by
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/sage5.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/sage5.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
Replying to nthiery:
Did you include your trac_12876reviewer.patch (which fixes those backticks) in between trac_12876_categoryfix_abstract_classntrel11521.patch and trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch?
Nope! I thought that your review patch replaces mine.
comment:39 in reply to: ↑ 37 Changed 8 years ago by
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
 Description modified (diff)
 Work issues changed from Fix the reviewnt 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 followup: ↓ 44 Changed 8 years ago by
PS: With
king@mpc622:/mnt/local/king/SAGE/stable/sage5.0.beta13/devel/sage$ hg qa trac_12808classcall_speedupfh.patch trac_12808_nested_class_cython.patch trac_12808classcall_cdef.patch trac12215_weak_cached_function.patch trac12215_segfault_fixes.patch trac_715_combined.patch trac_11521_homset_weakcache_combined.patch trac_12875categoryfix_abvar_homspacent.patch trac_12877categoryfor_more_rings_and_schemesnt.patch trac_12876_categoryfix_abstract_classntrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch
sage.categories.rings.Rings.HomCategory
exists and is empty, as it is supposed to be, I guess.
comment:42 followup: ↓ 43 Changed 8 years ago by
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
comment:44 in reply to: ↑ 41 ; followup: ↓ 45 Changed 8 years ago by
Replying to SimonKing:
PS: With
king@mpc622:/mnt/local/king/SAGE/stable/sage5.0.beta13/devel/sage$ hg qa trac_12808classcall_speedupfh.patch trac_12808_nested_class_cython.patch trac_12808classcall_cdef.patch trac12215_weak_cached_function.patch trac12215_segfault_fixes.patch trac_715_combined.patch trac_11521_homset_weakcache_combined.patch trac_12875categoryfix_abvar_homspacent.patch trac_12877categoryfor_more_rings_and_schemesnt.patch trac_12876_categoryfix_abstract_classntrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.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
comment:46 Changed 8 years ago by
 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/sage5.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
PS: The obvious fix is to change the expected result in the test...
comment:48 Changed 8 years ago by
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 followup: ↓ 50 Changed 8 years ago by
 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
comment:51 Changed 8 years ago by
 Work issues Fx doctests in sageinspect.py deleted
I forgot to remove the "work issues".
comment:52 Changed 8 years ago by
 Milestone changed from sage5.1 to sagepending
comment:53 Changed 8 years ago by
 Work issues set to commit message
The third patch needs a proper commit message.
comment:54 Changed 8 years ago by
 Milestone changed from sagepending to sage5.4
 Status changed from positive_review to needs_work
comment:55 Changed 8 years ago by
 Dependencies changed from #715, #11521, #12875, #12877, #12215 to #715, #11521, #12215
Changed 8 years ago by
Replacement of trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch wrt to the new patch versions at #715, #12313 etc.
comment:56 Changed 8 years ago by
 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_categoryfix_abstract_classskrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch
comment:57 Changed 8 years ago by
 Status changed from needs_review to needs_work
 Work issues changed from commit message to fix doctests and segfaults
With sage5.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/sage5.3.beta2/devel/sagemain/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 libcstart.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 8 years ago by
 Dependencies changed from #715, #11521, #12215 to #715, #11521, #12215, #12313
comment:59 Changed 8 years ago by
 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_categoryfix_abstract_classskrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch trac_12876_fix_infinite_polynomial_ring.patch
comment:60 followup: ↓ 61 Changed 8 years ago by
 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 8 years ago by
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 followups: ↓ 63 ↓ 64 Changed 8 years ago by
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 8 years ago by
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 ; followup: ↓ 65 Changed 8 years ago by
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)) Falseso 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 8 years ago by
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 8 years ago by
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_categoryfix_abstract_classskrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch trac_12876_fix_infinite_polynomial_ring.patch
Changed 8 years ago by
Do not weakcache things in infinite_polynomial_ring that are cached anyway. Skip more tests in Hecke homset's TestSuite
comment:67 Changed 8 years ago by
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_categoryfix_abstract_classskrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch trac_12876_fix_infinite_polynomial_ring.patch
comment:68 followup: ↓ 74 Changed 8 years ago by
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.
comment:69 Changed 8 years ago by
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 8 years ago by
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.
comment:71 followup: ↓ 76 Changed 8 years ago by
 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_categoryfix_abstract_classntrel11521reviewnt.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_categoryfix_abstract_classskrel11521.patch trac_12876reviewer.patch trac_12876_categoryfix_abstract_classntrel11521reviewnt.patch trac_12876_fix_infinite_polynomial_ring.patch trac_12876_another_reviewer.patch
comment:72 followups: ↓ 73 ↓ 75 Changed 8 years ago by
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 8 years ago by
comment:74 in reply to: ↑ 68 Changed 8 years ago by
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 8 years ago by
Replying to SimonKing:
Hence, apparently
Ring.__init__
is not called onPowerSeriesRing
. 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 ; followup: ↓ 77 Changed 8 years ago by
Replying to SimonKing:
I give it a positive review, modulo commit message of trac_12876_categoryfix_abstract_classntrel11521reviewnt.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 ; followup: ↓ 78 Changed 8 years ago by
Replying to nthiery:
Replying to SimonKing:
I give it a positive review, modulo commit message of trac_12876_categoryfix_abstract_classntrel11521reviewnt.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_categoryfix_abstract_classntrel11521reviewnt.patch
by, say #12876: Use generic code for sage.categories.rings.Rings.HomCategory
would be enough.
comment:78 in reply to: ↑ 77 Changed 8 years ago by
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_categoryfix_abstract_classntrel11521reviewnt.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 followup: ↓ 80 Changed 8 years ago by
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 8 years ago by
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 followup: ↓ 82 Changed 8 years ago by
 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
comment:82 in reply to: ↑ 81 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
 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/sage12876/sage/rings/polynomial/infinite_polynomial_ring.py # Killed/crashed sage t force_lib devel/sage12876/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 8 years ago by
I tried to stresstest 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 1166 1166 1167 1167 :: 1168 1168 1169 sage: os.path.realpath(tmpdir) == sageobj(r.getwd())1169 sage: os.path.realpath(tmpdir) == os.path.realpath(sageobj(r.getwd())) 1170 1170 True 1171 1171 """ 1172 1172 self.execute('setwd(%r)' % dir)
comment:86 Changed 8 years ago by
 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
Note: this might depend on #12875