Opened 8 years ago

Last modified 5 years ago

#12452 needs_work enhancement

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

Reported by: burcin Owned by: burcin
Priority: minor Milestone: sage-6.4
Component: symbolics Keywords: Cernay2012
Cc: fhivert, jpflori Merged in:
Authors: Burcin Erocal Reviewers: Jean-Pierre Flori, Florent Hivert
Report Upstream: N/A Work issues: devise potential use cases, speed regression
Branch: Commit:
Dependencies: Stopgaps:

Description

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.

Attachments (1)

trac_12452-subclass_expression.patch (32.2 KB) - added by burcin 8 years ago.

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by burcin

comment:1 Changed 8 years ago by burcin

  • Keywords Cernay2012 added
  • Status changed from new to needs_review

comment:2 follow-up: Changed 8 years ago by 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 ?

I'll run some tests now.

comment:3 in reply to: ↑ 2 Changed 8 years ago by burcin

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.

comment:4 follow-up: Changed 8 years ago by hivert

  • Reviewers set to Jean-Pierre Flori, Florent Hivert

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.

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

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.

comment:6 in reply to: ↑ 5 Changed 8 years ago by jpflori

  • Cc jpflori added
  • Status changed from needs_review to needs_work
  • Work issues set to devise potential use cases, speed regression

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.

comment:7 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:8 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.