Opened 8 years ago

Closed 8 years ago

Last modified 4 years ago

#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 nthiery)

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)

categories_testsuite-nt.patch (64.2 KB) - added by nthiery 8 years ago.
Patch 2: stronger category tests
trac_7921-categories_for_extension_types-nt.patch (82.7 KB) - added by nthiery 8 years ago.
Rebased and updated one doctest for 4.3.1 + micro fix in the primer. Apply only this one.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by nthiery

  • Cc roed added; roe removed
  • Description modified (diff)

comment:2 Changed 8 years ago by nthiery

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

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!

comment:3 Changed 8 years ago by nthiery

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

comment:4 Changed 8 years ago by nthiery

  • 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 8 years ago by nthiery

  • 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 8 years ago by nthiery

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.

Changed 8 years ago by nthiery

Patch 2: stronger category tests

comment:7 follow-up: Changed 8 years ago by robertwb

  • 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: Changed 8 years ago by robertwb

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: Changed 8 years ago by nthiery

  • 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: Changed 8 years ago by nthiery

  • 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 8 years ago by nthiery

Replying to nthiery:

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.

This is now #8001 (darn, missed, that was sooo close from #8000!)

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by robertwb

  • 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 8 years ago by nthiery

Thanks Robert for the quick review! I am looking forward feedback from practical uses :-)

Thanks also for the pointer to #383

Changed 8 years ago by nthiery

Rebased and updated one doctest for 4.3.1 + micro fix in the primer. Apply only this one.

comment:14 Changed 8 years ago by mvngu

  • Merged in set to sage-4.3.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 follow-up: Changed 4 years ago by 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 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"))
Last edited 4 years ago by saraedum (previous) (diff)

comment:16 in reply to: ↑ 15 Changed 4 years ago by nthiery

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

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 4 years ago by saraedum

Thanks for your explanation.

Note: See TracTickets for help on using tickets.