Opened 5 years ago

Closed 5 years ago

#21895 closed defect (fixed)

Better metaclass inference in dynamic classes

Reported by: saraedum Owned by:
Priority: major Milestone: sage-7.5
Component: categories Keywords: metaclass
Cc: nthiery, roed Merged in:
Authors: Julian Rüth Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 86b4868 (Commits, GitHub, GitLab) Commit: 86b4868e9deb41940e3bed2a3289276bd7557a52
Dependencies: Stopgaps:

Status badges

Description

When creating a dynamic class, (e.g., when creating an object in a category,) the following code is used to figure out the metaclass:

metaclass = DynamicMetaclass
# The metaclass of a class must derive from the metaclasses of its
# bases. The following handles the case where one of the base
# classes is a known Sage metaclass.  This approach won't scale
# well if we start using metaclasses seriously in Sage.
for base in bases:
    if isinstance(base, InheritComparisonClasscallMetaclass):
        metaclass = DynamicInheritComparisonClasscallMetaclass
    elif isinstance(base, ClasscallMetaclass):
        metaclass = DynamicClasscallMetaclass
    elif isinstance(base, InheritComparisonMetaclass):
metaclass = DynamicInheritComparisonMetaclass

This does not work if two bases have conflicting metaclasses, say InheritComparisonMetaclass and ClasscallMetaclass. As a result, one can currently not let a subclass of Morphism (which is a InheritComparisonMetaclass) inherit from UniqueRepresentation (a ClasscallMetaclass).

Change History (15)

comment:1 Changed 5 years ago by saraedum

  • Branch set to u/saraedum/better_metaclass_inference_in_dynamic_classes

comment:2 follow-up: Changed 5 years ago by saraedum

  • Commit set to c84f7ad60a8d8940e10109fc64a57662a59a8517

I fixed a real world problem that hit me when working on #21879 to showcase that this really works. I could also split this off into a different ticket and create a more artificial doctest for this, if the reviewer wants me to.


New commits:

e40f7a1Better inference of metaclass when creating dynamic classes
4c8be59Implement a morphism that inherits from UniqueRepresentation
6d4a953fix typo
726a9c3Do not skip test
4210389Verify that unique representation works
c84f7adNormalize arguments of AlgebraMorphism

comment:3 Changed 5 years ago by saraedum

  • Cc nthiery roed added

comment:4 Changed 5 years ago by saraedum

nthiery: I believe that you wrote the code that I quote in the summary?

comment:5 Changed 5 years ago by saraedum

  • Status changed from new to needs_review

comment:6 in reply to: ↑ 2 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to saraedum:

I fixed a real world problem that hit me when working on #21879 to showcase that this really works. I could also split this off into a different ticket and create a more artificial doctest for this, if the reviewer wants me to.

Please add such a doctest!

comment:7 Changed 5 years ago by jdemeyer

And I would also recommend to split off the changes which are unrelated to the topic of this ticket (the changes to AlgebraMorphism for example).

comment:8 Changed 5 years ago by git

  • Commit changed from c84f7ad60a8d8940e10109fc64a57662a59a8517 to 0f93b46ad07600c3e268644025261647087e9029

Branch pushed to git repo; I updated commit sha1. New commits:

0f93b46Add a doctest for 21895

comment:9 Changed 5 years ago by saraedum

  • Status changed from needs_work to needs_review

New commits:

0f93b46Add a doctest for 21895

New commits:

0f93b46Add a doctest for 21895

comment:10 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. Compare classes with is instead of ==.
  1. Move the changes to generic_basis_code.py to a new ticket.

comment:11 Changed 5 years ago by git

  • Commit changed from 0f93b46ad07600c3e268644025261647087e9029 to 86b4868e9deb41940e3bed2a3289276bd7557a52

Branch pushed to git repo; I updated commit sha1. New commits:

a4e567fuse "is" to compare types
86b4868remove algebra morphism changes

comment:12 Changed 5 years ago by saraedum

  • Status changed from needs_work to needs_review

New commits:

a4e567fuse "is" to compare types
86b4868remove algebra morphism changes

New commits:

a4e567fuse "is" to compare types
86b4868remove algebra morphism changes

comment:13 Changed 5 years ago by saraedum

I dropped the other change since I do not care for that particular case.

comment:14 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/saraedum/better_metaclass_inference_in_dynamic_classes to 86b4868e9deb41940e3bed2a3289276bd7557a52
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.