#7921 closed enhancement (fixed)
Categories for extension types via __getattr___
Reported by: | nthiery | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-4.3.2 |
Component: | categories | Keywords: | |
Cc: | sage-combinat, mhansen, robertwb, roed | Merged in: | sage-4.3.2.alpha0 |
Authors: | Nicolas M. Thiéry | Reviewers: | Robert Bradshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
With this patch, all parents and elements can inherit code from categories, even extension types. This includes in particular generic tests (see TestSuite?(...).run()):
sage: ZZ.category() Category of commutative rings sage: TestSuite(ZZ).run(verbose = True) running ._test_additive_associativity() . . . pass running ._test_an_element() . . . pass running ._test_associativity() . . . pass running ._test_element_pickling() . . . pass running ._test_not_implemented_methods() . . . pass running ._test_one() . . . pass running ._test_pickling() . . . pass running ._test_prod() . . . pass running ._test_some_elements() . . . pass running ._test_zero() . . . pass
It is to be expected that this will catch bugs in many places in the library. To start with, see #7922, #7929, #7945, #7946
See patch description for details.
Patch also available on the sage-combinat server, with a +category guard: http://combinat.sagemath.org/hgwebdir.cgi/patches/file/tip/trac_7921-categories_for_extension_types-nt.patch
Attachments (2)
Change History (19)
comment:1 Changed 7 years ago by
- Cc roed added; roe removed
- Description modified (diff)
comment:2 Changed 7 years ago by
- Description modified (diff)
- Status changed from new to needs_work
comment:3 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:4 Changed 7 years ago by
- Description modified (diff)
The patch passes all tests with sage 4.3 + sage-combinat patches merged in 4.3.1 on my machine. I'll run sage -t -long tonight and report.
comment:5 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Robert Bradshaw
Hi Robert; I improved a bit TestSuite? to catch more. Should I add a second patch to this one, or make it a separate ticket?
comment:6 Changed 7 years ago by
Robert: please let me know if I should open a separate ticket for the second patch. Florent can probably review it if this can save you some time.
comment:7 follow-up: ↓ 10 Changed 7 years ago by
- Status changed from needs_review to needs_info
The attribute lookup code looks good. Most of the other changes are minor, though changing loads/dumps to running tests is an independent change is seems.
sage/groups/group.pyx
def __call__(self, x): # NT: doesn't this get in the way of the coercion mechanism?
Groups are not yet converted over to the new coercion model, and are a mess in general.
In sage/modular/abvar/abvar.py, you removed the method but kept the docstring floating there. Those tests should be kept, but probably not put there.
sage/modules/free_module.py - It'd be good to test the category of non-vector space.
Could you explain the changes to sage/structure/sage_object.pyx?
comment:8 follow-up: ↓ 9 Changed 7 years ago by
I didn't look much at the second patch, but this should almost certainly be a second ticket.
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 7 years ago by
- Status changed from needs_info to needs_review
Replying to robertwb:
I didn't look much at the second patch, but this should almost certainly be a second ticket.
Ok, will do. Florent volunteered to review it, since it's mostly about testsuites and categories.
comment:10 in reply to: ↑ 7 ; follow-up: ↓ 12 Changed 7 years ago by
- Milestone set to sage-4.3.2
Replying to robertwb:
The attribute lookup code looks good. Most of the other changes are minor, though changing loads/dumps to running tests is an independent change is seems.
Well, I actually only added 2/3 of them, mostly as an attempt to catch possible introduced issues. The others were already there, and needed to be updated due to all the new (often failing) tests coming from categories.
sage/groups/group.pyx
def __call__(self, x): # NT: doesn't this get in the way of the coercion mechanism?Groups are not yet converted over to the new coercion model, and are a mess in general.
Ok. Shouldn't this def be removed, so as not to prevent groups inheriting from Group to progressively get converted to the coercion model? Sure, that should be a separate patch. As for the comment above: I just can't prevent myself from adding comments in the code when I stumble on strange stuff. I can remove it if you prefer.
In sage/modular/abvar/abvar.py, you removed the method but kept the docstring floating there. Those tests should be kept, but probably not put there.
Oops, fixed.
sage/modules/free_module.py - It'd be good to test the category of non-vector space.
Done.
Could you explain the changes to sage/structure/sage_object.pyx?
I improved the description on top of the patch, including a comment about this.
comment:11 in reply to: ↑ 9 Changed 7 years ago by
comment:12 in reply to: ↑ 10 ; follow-up: ↓ 13 Changed 7 years ago by
- Status changed from needs_review to positive_review
Replying to nthiery:
Replying to robertwb:
The attribute lookup code looks good. Most of the other changes are minor, though changing loads/dumps to running tests is an independent change is seems.
Well, I actually only added 2/3 of them, mostly as an attempt to catch possible introduced issues. The others were already there, and needed to be updated due to all the new (often failing) tests coming from categories.
OK.
sage/groups/group.pyx
def __call__(self, x): # NT: doesn't this get in the way of the coercion mechanism?Groups are not yet converted over to the new coercion model, and are a mess in general.
Ok. Shouldn't this def be removed, so as not to prevent groups inheriting from Group to progressively get converted to the coercion model?
Eventually, for sure.
Sure, that should be a separate patch. As for the comment above: I just can't prevent myself from adding comments in the code when I stumble on strange stuff. I can remove it if you prefer.
No, comments like this are good. I was just somewhat answering your question.
Could you explain the changes to sage/structure/sage_object.pyx?
I improved the description on top of the patch, including a comment about this.
Thanks. Look forward to being able to do stuff like this. On a somewhat related note, you might be interested in the binop decorator at #383.
comment:13 in reply to: ↑ 12 Changed 7 years ago by
Thanks Robert for the quick review! I am looking forward feedback from practical uses :-)
Thanks also for the pointer to #383
Changed 7 years ago by
Rebased and updated one doctest for 4.3.1 + micro fix in the primer. Apply only this one.
comment:14 Changed 7 years ago by
- Merged in set to sage-4.3.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:15 follow-up: ↓ 16 Changed 3 years ago by
I have a question regarding this old ticket. The following fails:
sage: a = ModularForms(11,4).an_element() sage: a._test_category()
The reason for this is that a
is not an extension class so _test_category
expects that the type of a
inherits from a.parent().category().element_class
. Now if I try to do such inheritance
class HeckeModuleElement(sage.modules.module_element.ModuleElement, HeckeModules.element_class):
I get a
metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its base
Anyway, I'm getting the feeling that such inheritance does not happen anywhere (partly because most element classes are extension classes). The methods from the element_class are available anyway (just like for extension classes). So is it safe to remove this check from _test_category
? I.e. should we change
if not is_extension_type(self.__class__): # For usual Python classes, that should be done with # standard inheritance tester.assert_(isinstance(self, self.parent().category().element_class)) else: # For extension types we just check that inheritance # occurs on a dummy attribute of Sets().ElementMethods tester.assert_(hasattr(self, "_dummy_attribute"))
into
tester.assert_(hasattr(self, "_dummy_attribute"))
comment:16 in reply to: ↑ 15 Changed 3 years ago by
Replying to saraedum:
I have a question regarding this old ticket. The following fails:
sage: a = ModularForms(11,4).an_element() sage: a._test_category()The reason for this is that
a
is not an extension class so_test_category
expects that the type ofa
inherits froma.parent().category().element_class
. Now if I try to do such inheritanceclass HeckeModuleElement(sage.modules.module_element.ModuleElement, HeckeModules.element_class):I get a
metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its base
If it's not an extension type, it really should inherit a.parent().category().element_class
.
Except in complicated cases (e.g. parents whose elements can belong to
several classes), this is taken care of automatically if a category is specified to
Parent.__init__
; otherwise one needs to use manually
_make_element_class
. See sage.categories.primer (better with #10963 applied) and the
documentation of Category for details.
Most likely the issue here is that the category is not specified properly.
Anyway, I'm getting the feeling that such inheritance does not happen anywhere (partly because most element classes are extension classes).
Most of my bread and butter element classes are *not* extension classes :-) Just for one example:
sage: x = CombinatorialFreeModule(QQ, [1,2,3]).an_element() sage: x.__class__ <class 'sage.combinat.free_module.CombinatorialFreeModule_with_category.element_class'> sage: x.__class__.__bases__ (sage.combinat.free_module.CombinatorialFreeModuleElement, sage.categories.vector_spaces.VectorSpaces.WithBasis.element_class)
So is it safe to remove this check from
_test_category
?
Please don't: so far failures here have always pointed to actual bugs.
Cheers,
Nicolas
comment:17 Changed 3 years ago by
Thanks for your explanation.
All tests seem to pass with it on 4.3; I still need to double check a couple things. It does change things in many places; so the best would be to integrate it in the early phase of 4.3.2 before it rots away.
Early feedback welcome!