Opened 3 years ago

Last modified 3 years ago

#20859 new enhancement

Simplify the logic handling the EvaluationMethods mixin class for Expression

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.3
Component: symbolics Keywords:
Cc: nthiery, tscrim Merged in:
Authors: Nicolas M. Thiéry Reviewers:
Report Upstream: N/A Work issues:
Branch: u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes (Commits) Commit: 5619a7d0f032f4273069166e6babf938b8c3f40a
Dependencies: #20825, #20686 Stopgaps:

Description


Change History (18)

comment:1 Changed 3 years ago by jdemeyer

  • Dependencies set to #20825, #20686

comment:2 Changed 3 years ago by jdemeyer

  • Branch set to u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes
  • Commit set to 5619a7d0f032f4273069166e6babf938b8c3f40a

New commits:

44e80b3EvaluationMethods should be a new-style class
9a1ff6eImprove getattr_from_other_class
744ffa6Test that static methods work
f5361d9Improve _sage_src_lines_()
782f14020686: minor comments improvements
dc84d88Remove comments about "private" attributes with two underscores
8a4f48dAbstract away category lookup in cdef method getattr_from_category
2e93afeMerge branch 'u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes' of trac.sagemath.org:sage into t/20686/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes
5619a7d20686: simplified the logic handling the EvaluationMethods mixin class for Expression's (internally backward incompatible)

comment:3 follow-up: Changed 3 years ago by jdemeyer

You really should use new-style classes. Old-style classes exist only for backwards compatibility and will be gone in Python 3.

comment:4 Changed 3 years ago by jdemeyer

I just moved your branch from #20686 here. Feel free to rewrite history to make it depend only on #20825.

comment:5 follow-up: Changed 3 years ago by jdemeyer

The use of cls.__dict__ seems to preclude the use of inheritance since cls.__dict__ does not do MRO lookup while dir(cls) does.

comment:6 Changed 3 years ago by nthiery

Thanks for creating the ticket and splitting of the commit! I am fine with this depending on #20686.

comment:7 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by nthiery

Replying to jdemeyer:

You really should use new-style classes. Old-style classes exist only for backwards compatibility and will be gone in Python 3.

Fun: from the same premises, we arrive at opposite conclusions :-)

Here is my logical chain:

In Python 3 we won't make the inheritance from object explicit: it would be redundant to write:

        class XXXMethods(object):

Furthermore, for our XXXMethods classes, it does not matter whether they are old style or new style classes (they just are bags of methods). Thus it feels natural to use right away the Python 3 idiom. That's what we have been doing in all the categories.

comment:8 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by nthiery

Replying to jdemeyer:

The use of cls.__dict__ seems to preclude the use of inheritance since cls.__dict__ does not do MRO lookup while dir(cls) does.

That's on purpose, for consistency with what we do in categories: the XXXMethods are meant to be pure mixins / bags of methods; they are not supposed to inherit from anything.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by jdemeyer

Replying to nthiery:

XXXMethods are meant to be pure mixins / bags of methods;

Given that it's a class, I would expect it to behave like a class. If you insist that it should not behave like a class, then at least make it explicit and invent a new metaclass BagOfMethods which disallows inheritance for example.

they are not supposed to inherit from anything.

How is a random Sage developer supposed to know that? That's one thing that I really don't like about the category framework in general: it makes several assumptions which work fine most of the time, but can bite you badly (the automatic binding is another such one). It almost feels like a slightly different programming language (that class is not really a Python class, it's just a dict).

comment:10 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by jdemeyer

Replying to nthiery:

Furthermore, for our XXXMethods classes, it does not matter whether they are old style or new style classes

It might matter in more places than you think. There will be some porting effort needed to transition from old-style classes to new-style classes (some issues came up in #20686). It would be better to do this now to avoid unexpected issues with Python 3.

For this reason, I am very against this change:

-    class EvaluationMethods(object):
+    class EvaluationMethods:

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by nthiery

Replying to jdemeyer:

Replying to nthiery:

Furthermore, for our XXXMethods classes, it does not matter whether they are old style or new style classes

It might matter in more places than you think. There will be some porting effort needed to transition from old-style classes to new-style classes (some issues came up in #20686). It would be better to do this now to avoid unexpected issues with Python 3.

For this reason, I am very against this change:

-    class EvaluationMethods(object):
+    class EvaluationMethods:

I already did tests with inheriting from object in some XXXMethods, and the category framework kept working the exact same way. I am therefore convinced there won't be anything to change for that specific aspect for Python 3.

Furthermore, the more consistent things will be across the library, the easier the porting will be.

Cheers,

Nicolas

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by jdemeyer

Replying to nthiery:

I already did tests with inheriting from object in some XXXMethods, and the category framework kept working the exact same way.

So you did some limited testing now and it worked. That's a good thing, but it doesn't guarantee that it will work in all cases now and in the future. As a general principle, we should try to move Sage as close to Python 3 as possible. Python 3 has only new-style classes, so we should use new-style classes.

Furthermore, the more consistent things will be across the library, the easier the porting will be.

True, but besides the point.

And of course, this just means that we should use new-style classes everywhere in Sage.

comment:13 in reply to: ↑ 9 ; follow-ups: Changed 3 years ago by nthiery

Replying to jdemeyer:

Given that it's a class, I would expect it to behave like a class. If you insist that it should not behave like a class, then at least make it explicit and invent a new metaclass BagOfMethods which disallows inheritance for example.

Using a metaclass would mean one more piece of purely technical syntax, which is one more chance for the programmer to forget something. Still you have a good point here: the infrastructure should check that XXXMethods inherits from nothing, and raise an explanatory error message if not.

How is a random Sage developer supposed to know that? That's one thing that I really don't like about the category framework in general: it makes several assumptions which work fine most of the time, but can bite you badly (the automatic binding is another such one). It almost feels like a slightly different programming language (that class is not really a Python class, it's just a dict).

That's the problem with every framework: a Django programmer needs to learn some about Django, etc, either from example by looking at existing code, or by reading the documentation.

Now does Sage really need such a framework? I am convinced enough about it to have invested altogether one solid year of work into it. Which does not mean I am not completely wrong :-)

I also believe in concise syntax with minimal boilerplate. Of course the price is additional complexity for those developers like you that not only use, but also work on the infrastructure itself.

Cheers,

Nicolas

comment:14 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by nthiery

Replying to jdemeyer:

And of course, this just means that we should use new-style classes everywhere in Sage.

Everywhere, unless we have a good reason to be convinced this does not make a difference.

Would we really gain something worth the trouble by adding an explicit inheritance from object in all 292 XXXMethods classes in Sage, and then removing them after the switch to Python3, with all the risks of induced syntactical conflicts, when we know that they are treated uniformly?

There are real issues in the switch to Python 3, and I believe this is not one.

Cheers,

comment:15 in reply to: ↑ 13 Changed 3 years ago by jdemeyer

Replying to nthiery:

Using a metaclass would mean one more piece of purely technical syntax, which is one more chance for the programmer to forget something. Still you have a good point here: the infrastructure should check that XXXMethods inherits from nothing, and raise an explanatory error message if not.

I'm not saying a metaclass is the right solution, it was just some proposal. That being said, you could think if there are other ways in which your "bag-of-methods" classes differ from "real" classes. Such differences might be handled cleanly by such metaclass. For example, you could use the metaclass to disable automatic binding of methods or to disallow instantiation of the class.

comment:16 in reply to: ↑ 13 Changed 3 years ago by jdemeyer

Replying to nthiery:

I also believe in concise syntax with minimal boilerplate.

I know :-) but sometimes that concise syntax just hides all kinds of stuff which is better made explicit. Like I said before: if I see a class, I expect it to behave like a class.

comment:17 in reply to: ↑ 14 Changed 3 years ago by jdemeyer

The hypothetical metaclass would also serve as entry point to documentation. If I see

class ElementMethods(BagOfStuff):
    ....

somewhere and want to understand what it does, I could do

sage: BagOfStuff?

which will hopefully explain that this class isn't really a class, but just some syntax to define a dict.

I don't know those XXXMethods classes well enough to say whether it's the right solution, but it's certainly something you should consider.

And this BagOfStuff would of course be a new-style class, rendering the other discussion obsolete :-)

comment:18 Changed 3 years ago by jdemeyer

I know I am repeating myself, but just to clear, I think there are two possible ways:

  1. Either you use a plain class XXXMethods but then it should behave like a class.
  1. Or it's something else, say class XXXMethods(BagOfStuff) and then you can have all kinds of strange behaviour that you document in BagOfStuff?.
Note: See TracTickets for help on using tickets.