Opened 10 years ago
Last modified 9 years ago
#12876 closed enhancement
Fix element and parent classes of Hom categories to be abstract, and simplify the Hom logic. — at Version 19
Reported by: | nthiery | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-5.11 |
Component: | categories | Keywords: | categories, Hom |
Cc: | sage-combinat, SimonKing | Merged in: | |
Authors: | Nicolas M. Thiéry | Reviewers: | Simon King |
Report Upstream: | N/A | Work issues: | Fix ring_refcount_dict problem |
Branch: | Commit: | ||
Dependencies: | #12875, #12877 | 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 (20)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Description modified (diff)
comment:3 Changed 10 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/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 10 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 10 years ago by
- Description modified (diff)
comment:6 Changed 10 years ago by
Oops. The updated patch includes two further hunks I had forgotten.
comment:7 Changed 10 years ago by
Arr, yet another missing hunk ... Time to go to bed!
comment:8 follow-up: ↓ 10 Changed 10 years ago by
- Dependencies set to #12877
comment:9 Changed 10 years ago by
- Cc SimonKing added
comment:10 in reply to: ↑ 8 Changed 10 years ago by
- Dependencies changed from #12877 to #12875, #12877
comment:11 follow-up: ↓ 13 Changed 10 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 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 10 years ago by
PS: I just verified that the problem also occurs without #12808.
comment:13 in reply to: ↑ 11 Changed 10 years ago by
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 10 years ago by
comment:15 Changed 10 years ago by
comment:16 Changed 10 years ago by
PS: I am just attempting a rebase, to see whether #11521 fixes the ring_refcount_dict problem.
comment:17 Changed 10 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 Changed 10 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 10 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_category-fix_abstract_class-nt-rel11521.patch
Note: this might depend on #12875