make it possible to inherit from sage.symbolic.expression.Expression.

[burcin]

Since most functions defined by symbolic expressions hardcode the type of the result as sage.symbolic.expression.Expression, it is not possible to inherit from the Expression class.

Attached patch tries to overcome this by using the type of the current object instead of Expression.

CC: @hivert @jpflori

Component: symbolics

Keywords: Cernay2012

Work Issues: devise potential use cases, speed regression

Author: Burcin Erocal

Reviewer: Jean-Pierre Flori, Florent Hivert

Issue created by migration from https://trac.sagemath.org/ticket/12452

[burcin]

Changed keywords from none to Cernay2012

[burcin]

comment:1

Attachment: trac_12452-subclass_expression.patch.gz

[jpflori]

comment:2

The code looks rather meaningful to me.

I'll just update some doc because it confused me a little in the class E part.

In this part can I replace the sef.!class! by E ? i.e. the meaning of your comment is that e + I would be of class E because of the default implementation of _new if its not overwritten here ?

I'll run some tests now.

[burcin]

comment:3

Replying to @jpflori:

The code looks rather meaningful to me.

I'll just update some doc because it confused me a little in the class E part.

In this part can I replace the sef.!class! by E ? i.e. the meaning of your comment is that e + I would be of class E because of the default implementation of _new if its not overwritten here ?

Yes, exactly. Feel free to change the documentation as you wish.

Thanks for looking into this.

[hivert]

Reviewer: Jean-Pierre Flori, Florent Hivert

[hivert]

comment:4

Thanks Burcin for this one.

I hope you now feel better and that you had a nice and safe way home.

The code looks good to me to and I got a all test passed on a sage-5.0.beta4. So I'm tempted to set a positive review. However, before doing that I'd like to answer the two following questions:

• as such, does it solve any problem we had with the former implantation of Expression ? For example I asked for this feature to be able to have indexed variables but I don't think I can do it with the current patch. Of course I realize that it won't be the right solution since we can go directly using PyNAC

• is there any measurable slowdown because of the extra work ?

So if you don't mind I'd like to experiment a little more before putting the positive review.

[burcin]

comment:5

Replying to @hivert:

I hope you now feel better and that you had a nice and safe way home.

Thanks. I'm back at work now. Unfortunately, I'll only be able to take a look at the indexed expressions (#11576) first week of March.

The code looks good to me to and I got a all test passed on a sage-5.0.beta4. So I'm tempted to set a positive review. However, before doing that I'd like to answer the two following questions:

• as such, does it solve any problem we had with the former implantation of Expression ? For example I asked for this feature to be able to have indexed variables but I don't think I can do it with the current patch. Of course I realize that it won't be the right solution since we can go directly using PyNAC

No, this will not solve your problem. It only allows one to carry around some information with symbolic expressions. Even then, to handle the propagation of that info, you'd have to implement many methods yourself.

I'm not convinced if there is a real use case for this actually. There were many requests on sage-support, but AFAICT, all those actually needed indexed expressions not their own expression class.

• is there any measurable slowdown because of the extra work ?

Yes, definitely:

http://groups.google.com/group/sage-support/msg/511d84f6d744621e

That was based on an earlier version of this patch.

So if you don't mind I'd like to experiment a little more before putting the positive review.

Sure. I appreciate a thorough review.

I'd also be happy to just leave this patch on trac until somebody comes up with a real use case.

[jpflori]

comment:6

Replying to @burcin:

No, this will not solve your problem. It only allows one to carry around some information with symbolic expressions. Even then, to handle the propagation of that info, you'd have to implement many methods yourself. I'm not convinced if there is a real use case for this actually. There were many requests on sage-support, but AFAICT, all those actually needed indexed expressions not their own expression class.

I thought about what Florent told me in the RER before I fell asleep, and it indeed looks completely non-trivial to build an actual useful extension of the Expression class.

In particular dealing with commutativity of the addition or things like that, in order not to restrict to additionning elements in the new subclass.

• is there any measurable slowdown because of the extra work ?

Yes, definitely: http://groups.google.com/group/sage-support/msg/511d84f6d744621e That was based on an earlier version of this patch.

That's quite bad. I'll have a look at the latest patch.

[jpflori]

Work Issues: devise potential use cases, speed regression