Bunch of small changes to sage/gsl/interpolation.pyx

50 views
Skip to first unread message

Joris Vankerschaver

unread,
Sep 25, 2012, 6:28:26 AM9/25/12
to sage-...@googlegroups.com
Hi everybody, 

I made two small changes to the Sage interface for cubic spline interpolation (found in sage/gsl/interpolation.pyx). The first is at 


and concerns a small bug which prevents splines from being recomputed when the interpolation points are changed. The patch just adds two lines to the relevant methods, along with a bunch of doctests, and should be a breeze to review.

The other patch is at 


and simply exposes two methods from GSL for spline integration and computation of the derivatives. 

One thing that I noticed when working on this patch is that Spline.list returns a reference to the spline interpolation points (this is actually noted in the documentation), allowing one again to change the interpolation points without forcing the spline to be recomputed. This issue was brought up in #12036 when doctest coverage was addressed, and I've opened a ticket about this at 


My question is, can I just change Spline.list() to return a copy of the list of interpolation points, rather than a reference? Strictly speaking, this would alter the interface and so a DepreciationWarning of some sort would be appropriate but I can't imagine that anybody is relying on this (buggy) behavior. Still, maybe there are other reasons for not making this change -- please let me know if this is the case.

With best wishes,
Joris Vankerschaver 

kcrisman

unread,
Sep 25, 2012, 8:27:01 AM9/25/12
to sage-...@googlegroups.com

One thing that I noticed when working on this patch is that Spline.list returns a reference to the spline interpolation points (this is actually noted in the documentation), allowing one again to change the interpolation points without forcing the spline to be recomputed. This issue was brought up in #12036 when doctest coverage was addressed, and I've opened a ticket about this at 


My question is, can I just change Spline.list() to return a copy of the list of interpolation points, rather than a reference? Strictly speaking, this would alter the interface and so a DepreciationWarning of some sort would be appropriate but I can't imagine that anybody is relying on this (buggy) behavior. Still, maybe there are other reasons for not making this change -- please let me know if this is the case.


This seems reasonable, in this case.  Changing wrong behavior is called fixing a bug and doesn't need deprecation.  That said, I'd be interested in knowing if other software *does* allow this sort of changing things, in which case it could be the case that people would be using it actively and it would need deprecation.

- kcrisman 

mhampton

unread,
Sep 25, 2012, 6:38:39 PM9/25/12
to sage-...@googlegroups.com

It seems safer to return a copy and not a reference.  The documentation does make it clear that it returns a reference, so it can't really be called a bug.  My guess is that there isn't any code out there that exploits that though, so I would vote to not bother with a deprecation warning.

-Marshall Hampton

Robert Bradshaw

unread,
Sep 25, 2012, 7:09:18 PM9/25/12
to sage-...@googlegroups.com
+1 to a copy. There could be another method, say points(), that
returns an (immutable) tuple. Even more sophisticated, one could
return a list with copy-on-write semantics.
> --
> You received this message because you are subscribed to the Google Groups
> "sage-devel" group.
> To post to this group, send email to sage-...@googlegroups.com.
> To unsubscribe from this group, send email to
> sage-devel+...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sage-devel?hl=en.
>
>

Joris Vankerschaver

unread,
Sep 28, 2012, 10:51:40 AM9/28/12
to sage-...@googlegroups.com
Thanks everybody! I've gone ahead and changed the list method to return a copy rather than a reference. This seemed like the most minimal change I could make.

Robert, how would I go about returning a list with copy-on-write semantics? Is there a standard container that does this?

All the best,
Joris
Reply all
Reply to author
Forward
0 new messages