Opened 11 years ago

Closed 11 years ago

#12036 closed enhancement (fixed)

improve doctest coverage of gsl/interpolation.pyx from 0% to 100%

Reported by: was Owned by: jason, jkantor
Priority: minor Milestone: sage-4.8
Component: numerical Keywords:
Cc: Merged in: sage-4.8.alpha2
Authors: William Stein Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Part of metaticket #12024.

Attachments (1)

trac_12036.patch (4.0 KB) - added by was 11 years ago.

Download all attachments as: .zip

Change History (6)

Changed 11 years ago by was

Attachment: trac_12036.patch added

comment:1 Changed 11 years ago by was

Status: newneeds_review

Note: This class does not support pickling, e.g., loads(dumps(spline(...))) just returns None!? And the data hiding isn't so good with it being possible to get a reference to the underlying list. Anyway, the point of this ticket is just to get coverage up to 100%, not to try to completely rewrite the code. Supporting pickling could be on another ticket. It would be an easy exercise in __reduce__.

comment:2 Changed 11 years ago by kcrisman

Authors: William Stein
Reviewers: Karl-Dieter Crisman
Status: needs_reviewneeds_info
sage -coverage sage/gsl/interpolation.pyx 
----------------------------------------------------------------------
sage/gsl/interpolation.pyx
ERROR: Please add a `TestSuite(s).run()` doctest.
SCORE sage/gsl/interpolation.pyx: 100% (9 of 9)
----------------------------------------------------------------------
sage -t  "devel/sage-main/sage/gsl/interpolation.pyx"       
	 [17.8 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 18.0 seconds

I assume the error about the testsuite would be part of this other ticket.

Also, the cdef methods are not tested (which I think is supposed to be okay), and neither is the __dealloc__ method, which apparently is ok since it didn't show up on sage -coverage.


Questions:

  • Should this be in the reference manual?
  • Should some of the examples in this patch (I really like the __setitem__ examples) be in the main thing, so that spline? is more useful?

If so, these might as well be taken care of now.

Otherwise looks good.

comment:3 Changed 11 years ago by was

Status: needs_infoneeds_review

Adding a TestSuite(s).run() run would be nice, but it will fail, since there is no support for pickling in this code. Adding this to reference manual would be nice too, but that's other work.

Regarding setitem, though it is perhaps interesting that this is supported, I doubt most people would even think to want to change the points that they are interpolating through after they create the spline.

Basically, I want modifications to code that I doctest to be minimal -- I'm adding tests to get the coverage score to 90% for sage-5.0. I don't want to introduce bugs. Say what you will about this patch, it can't introduce bugs because it only changes docstrings. I would like it refereed on those merits. Keep in mind that I'm not introducing a *new* class into Sage for the first time. If that were the case, then having TestSuite?, pickling support, etc., would be absolutely required.

Opening and resolving a new ticket to do everything you suggest will be easier now that this file has 100% coverage, since at least when adding some new code, you'll have some tests to run to reduce the chances the new code doesn't break things.

comment:4 Changed 11 years ago by kcrisman

Status: needs_reviewpositive_review

I wasn't too worried about TestSuite, just making sure (note I said "this other ticket", not meaning the current one). I'll take your point on the __setitem__ example as well, I don't use splines myself too much.

The reference manual issue is now #12045.

comment:5 Changed 11 years ago by jdemeyer

Merged in: sage-4.8.alpha2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.