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: |
Description
Part of metaticket #12024.
Attachments (1)
Change History (6)
Changed 11 years ago by
Attachment: | trac_12036.patch added |
---|
comment:1 Changed 11 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 11 years ago by
Authors: | → William Stein |
---|---|
Reviewers: | → Karl-Dieter Crisman |
Status: | needs_review → needs_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 thatspline?
is more useful?
If so, these might as well be taken care of now.
Otherwise looks good.
comment:3 Changed 11 years ago by
Status: | needs_info → needs_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
Status: | needs_review → positive_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
Merged in: | → sage-4.8.alpha2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
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__
.