[with patch, positive review] Implements a mantra for declaring abstract methods

[nthiery]

This patch implements a decorator tha can be used to declare a method that should be implemented by derived classes. This declaration should typically include documentation for the specification for this method.

The purpose is to enforce a consistent and visual mantra for such declarations. This is also used by #5891 for automated tests.

[was]

Feedback and suggestion for improvement on this policy are welcome!

sage: x = A() 
sage: x.my_method?
BOOM!

This must be fixed by improving sageinspect. That should be easy.

1) Trick to catch.

2) How do you get the docs on my_method though?

[nthiery]

Replying to was:

Feedback and suggestion for improvement on this policy are welcome!

{{{ sage: x = A() sage: x.my_method? BOOM! }}} This must be fixed by improving sageinspect. That should be easy. 1) Trick to catch. 2) How do you get the docs on my_method though?

I commented on those in the patch. I vote for integrating the code now, and improve it later when we will have more experience.

[ncalexan]

Making this fail on access is so introspection and command line unfriendly that I vote a *very strong* -1 to it.

[nthiery]

Replying to ncalexan:

Making this fail on access is so introspection and command line unfriendly that I vote a *very strong* -1 to it.

Thanks for the feedback!

Note that this is an exceptional situation: in principle, users are not supposed to encounter this situation (in a compiled language, the code would not even compile). In any cases, this will be trivial to change later.

Do you see any strong reason not to merge this patch as is, until we accumulate enough experience? (there are 20 patches piling upon this one).

[ncalexan]

Do you see any strong reason not to merge this patch as is, until we accumulate enough experience?

Yes. One cannot even loop over attributes of an object generically with such behaviour! I really can't stress how bad an idea I think this is.

[nthiery]

Replying to ncalexan:

Do you see any strong reason not to merge this patch as is, until we accumulate enough experience?

Yes. One cannot even loop over attributes of an object generically with such behaviour! I really can't stress how bad an idea I think this is.

So what? The class of this object is broken in the first place.

On the other hand, experience with MuPAD, tells that having early errors for such situations is much safer (note that a bound method need not be called immediately; instead it can be passed down to other functions and wreak havoc at a distant place later).

My point is: the issue is non trivial enough that it requires more experience and further debate (not just the two of us). And we do not have the time for this right now.

[ncalexan]

My point is: the issue is non trivial enough that it requires more experience and further debate (not just the two of us). And we do not have the time for this right now.

Wait, what is the rush?

So what? The class of this object is broken in the first place.

I think there is a significant difference between broken *functionality* and an object in the system that cannot be *interrogated* by the system.

On the other hand, experience with MuPAD, tells that having early errors for such situations is much safer (note that a bound method need not be called immediately; instead it can be passed down to other functions and wreak havoc at a distant place later).

This is often true. Can you guarantee that such a poisoned object can only be created by a user who "works hard" to do so? If that's the case then I remove whatever objections I have.

[nthiery]

Replying to ncalexan:

My point is: the issue is non trivial enough that it requires more experience and further debate (not just the two of us). And we do not have the time for this right now.

Wait, what is the rush?

The category patches + followers depend on this. Total 22 patches = 1.3 Mb :-)

So what? The class of this object is broken in the first place.

I think there is a significant difference between broken *functionality* and an object in the system that cannot be *interrogated* by the system.

Fair enough.

On the other hand, experience with MuPAD, tells that having early errors for such situations is much safer (note that a bound method need not be called immediately; instead it can be passed down to other functions and wreak havoc at a distant place later).

This is often true. Can you guarantee that such a poisoned object can only be created by a user who "works hard" to do so? If that's the case then I remove whatever objections I have.

In principle, such objects can only be created by:

• Instantiating an instance of an abstract class (which should not be permitted in the first place)
• Instantiating an instance of a concrete class which does not implement all the mandatory abstract methods of it super class
• Or shooting in your own foot, and creating the abstract_method yourself, as in the doctest.

I don't know how much "hard word" is the first point.

The second point can only occur if:

• either the user wrote the class himself (which sounds hard enough work)
• or if a developer did not do his job (or worst, the code went into sage through refereeing!)

Note that the category code includes checking methods that complains if a parent does not implement all the mandatory abstract methods. I plan to lift this up to SageObject?.

That being said, as a dev, I got caught myself at least once. So it's all a question of balance between two annoyances.

[ncalexan]

- Instantiating an instance of an abstract class (which should not be permitted in the first place)

Why not make init (or new) raise NotImplementedError??

- Instantiating an instance of a concrete class which does not implement all the mandatory abstract methods of it super class - Or shooting in your own foot, and creating the abstract_method yourself, as in the doctest.

These two can never be prevented against. So I'm more okay with this than I was.

[nthiery]

Replying to ncalexan:

- Instantiating an instance of an abstract class (which should not be permitted in the first place)

Why not make init (or new) raise NotImplementedError??

Because abstract methods are perfectly valid in an abstract class. Actually, the user will want to use introspection to query for their doc, ...

- Instantiating an instance of a concrete class which does not implement all the mandatory abstract methods of it super class - Or shooting in your own foot, and creating the abstract_method yourself, as in the doctest.

These two can never be prevented against. So I'm more okay with this than I was.

[ncalexan]

Replying to nthiery:

Replying to ncalexan:

- Instantiating an instance of an abstract class (which should not be permitted in the first place)

Why not make init (or new) raise NotImplementedError??

Because abstract methods are perfectly valid in an abstract class. Actually, the user will want to use introspection to query for their doc, ...

This seems wrong to me. init is only called on instance creation. Raising NotImplementedError? in instance creation just means you can't create an instance. How does this have anything to do with abstract methods or introspection?

[nthiery]

Replying to ncalexan:

Replying to nthiery:

Replying to ncalexan:

- Instantiating an instance of an abstract class (which should not be permitted in the first place)

Why not make init (or new) raise NotImplementedError??

Because abstract methods are perfectly valid in an abstract class. Actually, the user will want to use introspection to query for their doc, ...

This seems wrong to me. init is only called on instance creation. Raising NotImplementedError? in instance creation just means you can't create an instance. How does this have anything to do with abstract methods or introspection?

Oh, sorry, I got completely confused. I though you were speaking about the init of the AbstractMethod? class.

Yes, adding an init or new to abstract classes, so as to prevent element creation would sound reasonable. And we typically would want to do this by having and @abstract_class decorator. To be kept in mind for the future.

[saliola]

Here is a second opinion.

I really, really like the idea of adding such a decorator! And for the record, under normal circumstances, I would have asked for a full design discussion to take place on sage-devel since I share many of the above concerns and because I think that there are many subtle design issues that need to be addressed. (We'll have to have that discussion later, it seems.) However, in light of the decision to get the category theory code into Sage quickly, I am going to reluctantly suggest it be included in Sage as is.

Here is a list of things that I don't like (most have been mentioned above):

• raising NotImplementedError when an abstract method is accessed
• the error message is not useful
• returning NotImplemented for an optional abstract class
• can't access the documentation for an abstract method
• that hasattr(x, 'my_method') returns False for abstract methods and True for optional abstract methods
• can't determine what the undefined abstract methods of an object are

None of these are strong reasons against inclusion, I guess, since it might be just a matter of opinion, or that I haven't thought enough about the subtleties.

[nthiery]

Replying to saliola:

Here is a second opinion. I really, really like the idea of adding such a decorator! And for the record, under normal circumstances, I would have asked for a full design discussion to take place on sage-devel since I share many of the above concerns and because I think that there are many subtle design issues that need to be addressed. (We'll have to have that discussion later, it seems.)

I tried to advertise the feature to start such a discussion, but there was not much reaction. I think we first need to accumulate experience by using the current thing, and then discuss again. Release early, release often.

However, in light of the decision to get the category theory code into Sage quickly, I am going to reluctantly suggest it be included in Sage as is.

Thanks.

Here is a list of things that I don't like (most have been mentioned above): - raising NotImplementedError when an abstract method is accessed - the error message is not useful - returning NotImplemented for an optional abstract class - can't access the documentation for an abstract method - that hasattr(x, 'my_method') returns False for abstract methods and True for optional abstract methods - can't determine what the undefined abstract methods of an object are

Thanks for the feedback! I am also not yet happy about the current state. So do you have suggestions for what to return in both cases? What mantra would you like to see to test whether an optional abstract method has, or not, been implemented?

Feel free to include those suggestions in the doc via a reviewers patch, so that users run into them in the future.

[saliola]

Hello,

Replying to nthiery:

Replying to saliola:

Here is a second opinion. I really, really like the idea of adding such a decorator! And for the record, under normal circumstances, I would have asked for a full design discussion to take place on sage-devel since I share many of the above concerns and because I think that there are many subtle design issues that need to be addressed. (We'll have to have that discussion later, it seems.)

I tried to advertise the feature to start such a discussion, but there was not much reaction. I think we first need to accumulate experience by using the current thing, and then discuss again. Release early, release often.

I definitely missed this. I'm behind in reading the sage-devel emails.

Here is a list of things that I don't like (most have been mentioned above): - raising NotImplementedError when an abstract method is accessed - the error message is not useful - returning NotImplemented for an optional abstract class - can't access the documentation for an abstract method - that hasattr(x, 'my_method') returns False for abstract methods and True for optional abstract methods - can't determine what the undefined abstract methods of an object are

Thanks for the feedback! I am also not yet happy about the current state.

You mentioned that in the doc and in the above comments, and I must admit that this is one of the things that makes me more comfortable with including this into Sage. I know that you'll be around to improve it in the future, and that the design isn't complete yet.

So do you have suggestions for what to return in both cases? What mantra would you like to see to test whether an optional abstract method has, or not, been implemented?

You can't really tell from what I wrote in my last comment, but I spent almost 3 hours looking at this last night.

I thought about what I like and dislike about the existence of such a decorator. I really like the idea, but found myself wondering how much better is this, really, than good documentation? Will this encourage a developer to be lazy in documenting what needs to be implemented by subclasses? I strongly suspect this will be the case, which is fine as long as there is support for easily determining which methods of a class need to be implemented (I don't want to have to grep for abstract_method in the source). I wondered how I would use this as a developer of a subclass? I mean, given an object x, is there a way to tell which abstract methods of x need to be implemented without grepping the source? This is where introspection would normally come in (see below). And, as the developer of the subclass, it would be best to have x.my_method? return a docstring that says what the implementation should do (this is already mentioned in the docstring as a future improvement).

I wondered about, as ncalexan suggested, checking whether abstract methods are implemented at instance creation. This is exactly what happens now with CombinatorialAlgebra(QQ), but I find that this puts too much power in the hands of the developer since a user might want to use a class for something that doesn't require a particular abstract method at all.

I wondered about different mantras. If no error is returned on accessing elements, then one can use introspection to find all the abstract methods of an object.

sage: [meth for meth in dir(x) if type(x.meth, AbstractMethod)]

(The above currently doesn't work because x.my_method raises an error.)

I also wondered whether some methods might be decorated with abstract_method, but have a default implementation. I'm thinking of a decorator that signals to the user/developer that even though that particular method is implemented, it would be vastly better (for speed/memory/...) to provide a better implementation if one exists. I'm think about some of the methods of CombinatorialClass/EnumeratedSet, in particular. Perhaps this is beyond the scope of this decorator though, since an abstract method seems to be just dummy code usually.

Personally, I am leaning towards the idea of raising a NotImplementedError upon calling the method instead of upon accessing it, together with a good error message that tells the user that this method needs to be defined in order for what they just asked for to work. I haven't thought through this enough to be convinced that this is my preferred approach, and there might be some fine points I haven't thought of that makes this unappealing.

So, lots of thoughts, not many conclusions.

I decided to keep my last post short and to the point because I knew this type of post would be lengthy, but you asked!

Feel free to include those suggestions in the doc via a reviewers patch, so that users run into them in the future.

I think it isn't necessary since you have adequately stated in the documentation that it is just the beginning. And it also lists several todo's, one of which is to scour the web for other implementations/ideas and merge them. Anyhow, as I said above, I have arrived at no conclusions yet, just preferences.

[robertwb]

Though the interface hasn't completely settled, I think its stable enough to go in and worth starting to use. I'm opening a new ticket #6400 to review all the TODO's in this one.

[nthiery]

Hi Franco!

Replying to saliola:

Replying to nthiery:

Replying to saliola:

Here is a second opinion. I really, really like the idea of adding such a decorator! And for the record, under normal circumstances, I would have asked for a full design discussion to take place on sage-devel since I share many of the above concerns and because I think that there are many subtle design issues that need to be addressed. (We'll have to have that discussion later, it seems.)

I tried to advertise the feature to start such a discussion, but there was not much reaction. I think we first need to accumulate experience by using the current thing, and then discuss again. Release early, release often.

I definitely missed this. I'm behind in reading the sage-devel emails.

Oops, that was in my dreams. I only advertised it oraly at SD15. You should have been there :-)

You mentioned that in the doc and in the above comments, and I must admit that this is one of the things that makes me more comfortable with including this into Sage. I know that you'll be around to improve it in the future, and that the design isn't complete yet.

:-)

You can't really tell from what I wrote in my last comment, but I spent almost 3 hours looking at this last night.

Great.

I thought about what I like and dislike about the existence of such a decorator. I really like the idea, but found myself wondering how much better is this, really, than good documentation? Will this encourage a developer to be lazy in documenting what needs to be implemented by subclasses? I strongly suspect this will be the case, which is fine as long as there is support for easily determining which methods of a class need to be implemented (I don't want to have to grep for abstract_method in the source). I wondered how I would use this as a developer of a subclass? I mean, given an object x, is there a way to tell which abstract methods of x need to be implemented without grepping the source? This is where introspection would normally come in (see below). And, as the developer of the subclass, it would be best to have x.my_method? return a docstring that says what the implementation should do (this is already mentioned in the docstring as a future improvement).

Besides the nice visual effect, allowing for automatic checks is precisely the purpose of @abstract_method. This is already in #6343 (please update to the latest Sage-Combinat in an hour or so to get the exact syntax below):

sage: TestSuite?(ZZ).run(verbose = True) running ._test_not_implemented_methods() ... done running ._test_pickling() ... done

I wondered about, as ncalexan suggested, checking whether abstract methods are implemented at instance creation. This is exactly what happens now with CombinatorialAlgebra(QQ), but I find that this puts too much power in the hands of the developer since a user might want to use a class for something that doesn't require a particular abstract method at all.

That's sensible for Parents, but would not be reasonable for elements that could be created by the millions.

I wondered about different mantras. If no error is returned on accessing elements, then one can use introspection to find all the abstract methods of an object. {{{ sage: [meth for meth in dir(x) if type(x.meth, AbstractMethod?)] }}} (The above currently doesn't work because x.my_method raises an error.)

Note: you can do the same on x.class, which is what .test_not_implemented_methods does

I also wondered whether some methods might be decorated with abstract_method, but have a default implementation. I'm thinking of a decorator that signals to the user/developer that even though that particular method is implemented, it would be vastly better (for speed/memory/...) to provide a better implementation if one exists. I'm think about some of the methods of CombinatorialClass/EnumeratedSet, in particular. Perhaps this is beyond the scope of this decorator though, since an abstract method seems to be just dummy code usually.

I definitely would like this as well! Probably under a different name though, like @dummy_method / @default_method, since the purpose is a bit different.

Personally, I am leaning towards the idea of raising a NotImplementedError upon calling the method instead of upon accessing it, together with a good error message that tells the user that this method needs to be defined in order for what they just asked for to work. I haven't thought through this enough to be convinced that this is my preferred approach, and there might be some fine points I haven't thought of that makes this unappealing.

Ok. Let's see how this works in practice.

So, lots of thoughts, not many conclusions. I decided to keep my last post short and to the point because I knew this type of post would be lengthy, but you asked!

I did! Thanks very much for taking the time typing it up.

Cheers,