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)
Change History (11)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Keywords Cernay2012 added
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 8 years ago by
comment:3 in reply to: ↑ 2 Changed 8 years ago by
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: ↓ 5 Changed 8 years ago by
- 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: ↓ 6 Changed 8 years ago by
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
- 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
- Milestone changed from sage-5.11 to sage-5.12
comment:8 Changed 6 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:9 Changed 6 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 5 years ago by
- Milestone changed from sage-6.3 to sage-6.4
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.