Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
williamstein opened this issue Nov 15, 2011 · 7 comments
Closed

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

williamstein opened this issue Nov 15, 2011 · 7 comments

Comments

@williamstein
Copy link
Contributor

Part of metaticket #12024.

Component: numerical

Author: William Stein

Reviewer: Karl-Dieter Crisman

Merged: sage-4.8.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/12036

@williamstein
Copy link
Contributor Author

comment:1

Attachment: trac_12036.patch.gz

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__.

@kcrisman
Copy link
Member

comment:2
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.

@kcrisman
Copy link
Member

Author: William Stein

@kcrisman
Copy link
Member

Reviewer: Karl-Dieter Crisman

@williamstein
Copy link
Contributor Author

comment:3

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.

@kcrisman
Copy link
Member

comment:4

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.

@jdemeyer
Copy link

Merged: sage-4.8.alpha2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants