Sage: Ticket #18257: fix symbolic/pynac.pyx doctests
https://trac.sagemath.org/ticket/18257
<p>
Several doctests in <code>symbolic/pynac.pyx</code> iterate over <code>range(get_ginac_serial(), get_ginac_serial()+100)</code>. You guess it (sigh), <code>100</code> is arbitrary and, right now with some new tickets that introduce new functions, it has become too small, leading to unrelated doctest fails in <code>symbolic/pynac.pyx</code>. This ticket should make sure that this does not happen again.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/18257
Trac 1.1.6kcrismanFri, 22 May 2015 12:48:55 GMTpriority changed; cc set
https://trac.sagemath.org/ticket/18257#comment:1
https://trac.sagemath.org/ticket/18257#comment:1
<ul>
<li><strong>priority</strong>
changed from <em>major</em> to <em>minor</em>
</li>
<li><strong>cc</strong>
<em>kcrisman</em> added
</li>
</ul>
<p>
I agree that this is worth fixing permanently, but unless there is an obvious fix we shouldn't let this hold up adding more symbolic functions.
</p>
TicketrwsMon, 25 May 2015 08:37:52 GMT
https://trac.sagemath.org/ticket/18257#comment:2
https://trac.sagemath.org/ticket/18257#comment:2
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18257#comment:1" title="Comment 1">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
I agree that this is worth fixing permanently, but unless there is an obvious fix we shouldn't let this hold up adding more symbolic functions.
</p>
</blockquote>
<p>
This is not really difficult.
</p>
<p>
The central database is a dictionary globally defined in <code>symbolic/function.pyx</code> named <code>sfunction_serial_dict</code>. It has pairs with key of type <code>unsigned</code> and values of <code>Function</code> objects (which can be <code>GinacFunctions</code> or <code>BuiltinFunctions</code>). Accordingly this database is filled with all known Pynac <code>function</code>s (which have the <code>GinacFunction</code> façade) first and then those defined via <code>BuiltinFunction</code>. The <code>unsigned</code> key is the serial number which, on each new function registration, is incremented by one, namely in <code>GiNaC::function::register_new()</code>(<a class="ext-link" href="https://github.com/pynac/pynac/blob/master/ginac/function.cpp#L1445"><span class="icon"></span>https://github.com/pynac/pynac/blob/master/ginac/function.cpp#L1445</a>) by just adding to the Pynac registry and reporting its size minus one. After the GiNaC functions have been added, the global <code>GINAC_FN_SERIAL</code> is set which is then accessed via <code>get_ginac_serial()</code> to get the start key of the Sage defined functions.
</p>
<p>
There is a problem if ever a function is deregistered. Can this happen? No mechanism in Pynac exists for this at the moment, so it's not a practical consideration.
</p>
<p>
So the solution is easy: instead of simply adding <code>100</code> to the start point, we try all keys from the start key to the latest key used---this need not be a static variable set on registration. Since functions are never deregistered the latest is, again, just the size of the GiNaC registry minus one. This can be had from a call to <code>g_registered_functions().size()</code> in <code>symbolic/pynac.pyx</code>.
</p>
<pre class="wiki">cdef get_fn_serial_c():
return g_registered_functions().size()
def get_fn_serial():
return get_fn_serial_c()
sage: from sage.symbolic.pynac import get_fn_serial
sage: get_fn_serial()
127
sage: from sage.symbolic.pynac import get_ginac_serial
sage: get_ginac_serial()
40
sage: from sage.symbolic.function import get_sfunction_from_serial
sage: for i in range(get_ginac_serial(), get_fn_serial()):
print get_sfunction_from_serial(i)
... // prints all known functions
sage: print get_sfunction_from_serial(get_fn_serial()+1)
None
</pre>
TicketrwsMon, 25 May 2015 09:08:45 GMT
https://trac.sagemath.org/ticket/18257#comment:3
https://trac.sagemath.org/ticket/18257#comment:3
<p>
As they say the first thing that goes down the drain is the planning. The above solution does not account for user defined Python functions that do not inherit from <code>BuiltinFunction</code> or <code>Function</code>.
</p>
TicketrwsMon, 25 May 2015 13:37:41 GMTbranch set
https://trac.sagemath.org/ticket/18257#comment:4
https://trac.sagemath.org/ticket/18257#comment:4
<ul>
<li><strong>branch</strong>
set to <em>u/rws/fix_symbolic_pynac_pyx_doctests</em>
</li>
</ul>
TicketrwsMon, 25 May 2015 13:38:55 GMTpriority, status, milestone changed; commit set
https://trac.sagemath.org/ticket/18257#comment:5
https://trac.sagemath.org/ticket/18257#comment:5
<ul>
<li><strong>priority</strong>
changed from <em>minor</em> to <em>major</em>
</li>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>13ba99da6b01b2b94ee67a50169567def261898b</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-6.7</em> to <em>sage-6.8</em>
</li>
</ul>
<p>
No, that was an off-by-one error. Please review.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=13ba99da6b01b2b94ee67a50169567def261898b"><span class="icon"></span>13ba99d</a></td><td><code>18257: fix symbolic/pynac.pyx doctests</code>
</td></tr></table>
TicketkcrismanThu, 28 May 2015 15:14:24 GMT
https://trac.sagemath.org/ticket/18257#comment:6
https://trac.sagemath.org/ticket/18257#comment:6
<p>
Trivial - duplicate lines in test
</p>
<pre class="wiki">+ sage: from sage.symbolic.pynac import get_fn_serial
+ sage: from sage.symbolic.function import get_sfunction_from_serial
+ sage: from sage.symbolic.pynac import get_fn_serial
</pre><p>
This patch seems reasonable. What was <a class="ticket" href="https://trac.sagemath.org/ticket/18257#comment:3" title="Comment 3">comment:3</a> about? I sense that it was a red herring, but if it could be useful for test-driving this I would love to try it. I assume that since this isn't actually called in Sage but is really just for doctesting, it shouldn't have any speed implications.
</p>
TicketgitFri, 29 May 2015 07:11:35 GMTcommit changed
https://trac.sagemath.org/ticket/18257#comment:7
https://trac.sagemath.org/ticket/18257#comment:7
<ul>
<li><strong>commit</strong>
changed from <em>13ba99da6b01b2b94ee67a50169567def261898b</em> to <em>52ccd47dee6b0cb6da2ad6f3855942c84a36ac73</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=52ccd47dee6b0cb6da2ad6f3855942c84a36ac73"><span class="icon"></span>52ccd47</a></td><td><code>18257: fix typo</code>
</td></tr></table>
TicketrwsFri, 29 May 2015 07:12:28 GMTauthor set
https://trac.sagemath.org/ticket/18257#comment:8
https://trac.sagemath.org/ticket/18257#comment:8
<ul>
<li><strong>author</strong>
set to <em>Ralf Stephan</em>
</li>
</ul>
TicketrwsFri, 29 May 2015 07:13:29 GMT
https://trac.sagemath.org/ticket/18257#comment:9
https://trac.sagemath.org/ticket/18257#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/18257#comment:6" title="Comment 6">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
... What was <a class="ticket" href="https://trac.sagemath.org/ticket/18257#comment:3" title="Comment 3">comment:3</a> about? I sense that it was a red herring, but if it could be useful for test-driving this I would love to try it. I assume that since this isn't actually called in Sage but is really just for doctesting, it shouldn't have any speed implications.
</p>
</blockquote>
<p>
It's taken care of in the doctests.
</p>
TicketkcrismanFri, 29 May 2015 13:09:35 GMT
https://trac.sagemath.org/ticket/18257#comment:10
https://trac.sagemath.org/ticket/18257#comment:10
<p>
I'm not sure why but I still get
</p>
<pre class="wiki">sage -t src/sage/symbolic/pynac.pyx
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 512, in sage.symbolic.pynac.py_latex_function_pystring
Failed example:
get_sfunction_from_serial(i) == foo
Expected:
True
Got:
False
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 514, in sage.symbolic.pynac.py_latex_function_pystring
Failed example:
py_latex_function_pystring(i, (x,y^z))
Expected:
'my args are: x, y^z'
Got:
'\\mathrm{bar}\\left(x, y^{z}\\right)'
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 603, in sage.symbolic.pynac.py_print_fderivative_for_doctests
Failed example:
get_sfunction_from_serial(i) == foo
Expected:
True
Got:
False
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 605, in sage.symbolic.pynac.py_print_fderivative_for_doctests
Failed example:
py_print_fderivative(i, (0, 1, 0, 1), (x, y^z))
Expected:
D[0, 1, 0, 1]func_with_args(x, y^z)
Got:
D[0, 1, 0, 1](foo)(x, y^z)
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 665, in sage.symbolic.pynac.py_latex_fderivative_for_doctests
Failed example:
get_sfunction_from_serial(i) == foo
Expected:
True
Got:
False
**********************************************************************
File "src/sage/symbolic/pynac.pyx", line 667, in sage.symbolic.pynac.py_latex_fderivative_for_doctests
Failed example:
py_latex_fderivative(i, (0, 1, 0, 1), (x, y^z))
Expected:
D[0, 1, 0, 1]func_with_args(x, y^z)
Got:
D[0, 1, 0, 1]\left(\mathrm{bar}\right)\left(x, y^{z}\right)
**********************************************************************
</pre><p>
when I have both this ticket and <a class="closed ticket" href="https://trac.sagemath.org/ticket/15024" title="enhancement: Add Hankel functions and make spherical Bessel and Hankel functions ... (closed: fixed)">#15024</a> together. Did I do something wrong? (Note: this is based off of 6.8.beta1, if it matters.)
</p>
TicketrwsFri, 29 May 2015 15:05:54 GMT
https://trac.sagemath.org/ticket/18257#comment:11
https://trac.sagemath.org/ticket/18257#comment:11
<p>
Cannot confirm in a first attempt (I branched from <a class="closed ticket" href="https://trac.sagemath.org/ticket/18257" title="defect: fix symbolic/pynac.pyx doctests (closed: fixed)">#18257</a>, merged <a class="closed ticket" href="https://trac.sagemath.org/ticket/15024" title="enhancement: Add Hankel functions and make spherical Bessel and Hankel functions ... (closed: fixed)">#15024</a> with <code>git trac pull</code>, then merged develop).
</p>
TicketrwsFri, 29 May 2015 15:10:09 GMT
https://trac.sagemath.org/ticket/18257#comment:12
https://trac.sagemath.org/ticket/18257#comment:12
<p>
Same branch does work with pynac-0.3.9 (in case you left that installed and that was the difference between us).
</p>
TicketrwsFri, 29 May 2015 15:12:50 GMT
https://trac.sagemath.org/ticket/18257#comment:13
https://trac.sagemath.org/ticket/18257#comment:13
<p>
Your line numbers also don't match <a class="closed ticket" href="https://trac.sagemath.org/ticket/18257" title="defect: fix symbolic/pynac.pyx doctests (closed: fixed)">#18257</a>.
</p>
TicketkcrismanFri, 29 May 2015 18:53:22 GMT
https://trac.sagemath.org/ticket/18257#comment:14
https://trac.sagemath.org/ticket/18257#comment:14
<p>
Okay, I'll trash the branches I made locally and try again - may not be immediately, I'm at a conference.
</p>
TicketkcrismanMon, 01 Jun 2015 14:18:40 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/18257#comment:15
https://trac.sagemath.org/ticket/18257#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Karl-Dieter Crisman</em>
</li>
</ul>
<p>
I must have either not merged this one or perhaps in the wrong order or something... Anyway, all systems are go, and as expected <a class="closed ticket" href="https://trac.sagemath.org/ticket/15024" title="enhancement: Add Hankel functions and make spherical Bessel and Hankel functions ... (closed: fixed)">#15024</a> fails without this and works with it. Great!
</p>
TicketrwsMon, 01 Jun 2015 14:36:06 GMTstatus changed
https://trac.sagemath.org/ticket/18257#comment:16
https://trac.sagemath.org/ticket/18257#comment:16
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hold on. I have a necessary addition because of a commit in Pynac where I removed the registry entries for some defunct functions which makes two of the doctest here fail. This would fit in nicely here.
</p>
TicketgitMon, 01 Jun 2015 14:48:59 GMTcommit changed
https://trac.sagemath.org/ticket/18257#comment:17
https://trac.sagemath.org/ticket/18257#comment:17
<ul>
<li><strong>commit</strong>
changed from <em>52ccd47dee6b0cb6da2ad6f3855942c84a36ac73</em> to <em>557badf1a587f24e500dd12196b14bf59a144dd1</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=557badf1a587f24e500dd12196b14bf59a144dd1"><span class="icon"></span>557badf</a></td><td><code>adjust doctest because of upstream changes</code>
</td></tr></table>
TicketrwsMon, 01 Jun 2015 14:50:28 GMTstatus changed
https://trac.sagemath.org/ticket/18257#comment:18
https://trac.sagemath.org/ticket/18257#comment:18
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Could you please have a last quick look?
</p>
TicketkcrismanMon, 01 Jun 2015 14:57:52 GMT
https://trac.sagemath.org/ticket/18257#comment:19
https://trac.sagemath.org/ticket/18257#comment:19
<p>
I don't understand why this fails, though? (I mean it shouldn't currently, nothing failed for me.) Surely it would be better to put that commit with the next Pynac upgrade ticket? Easier for the reviewer to test, then (i.e. first upgrade Pynac, run tests (fail), then add branch, watch tests pass).
</p>
TicketrwsMon, 01 Jun 2015 15:21:59 GMTbranch changed
https://trac.sagemath.org/ticket/18257#comment:20
https://trac.sagemath.org/ticket/18257#comment:20
<ul>
<li><strong>branch</strong>
changed from <em>u/rws/fix_symbolic_pynac_pyx_doctests</em> to <em>u/rws/18257</em>
</li>
</ul>
TicketrwsMon, 01 Jun 2015 15:22:30 GMTstatus, commit changed
https://trac.sagemath.org/ticket/18257#comment:21
https://trac.sagemath.org/ticket/18257#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>commit</strong>
changed from <em>557badf1a587f24e500dd12196b14bf59a144dd1</em> to <em>52ccd47dee6b0cb6da2ad6f3855942c84a36ac73</em>
</li>
</ul>
TicketvbraunTue, 02 Jun 2015 20:18:15 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/18257#comment:22
https://trac.sagemath.org/ticket/18257#comment:22
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>u/rws/18257</em> to <em>52ccd47dee6b0cb6da2ad6f3855942c84a36ac73</em>
</li>
</ul>
Ticket