Opened 6 years ago
Closed 6 years ago
#18257 closed defect (fixed)
fix symbolic/pynac.pyx doctests
Reported by: | rws | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | symbolics | Keywords: | |
Cc: | kcrisman | Merged in: | |
Authors: | Ralf Stephan | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | 52ccd47 (Commits, GitHub, GitLab) | Commit: | 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73 |
Dependencies: | Stopgaps: |
Description
Several doctests in symbolic/pynac.pyx
iterate over range(get_ginac_serial(), get_ginac_serial()+100)
. You guess it (sigh), 100
is arbitrary and, right now with some new tickets that introduce new functions, it has become too small, leading to unrelated doctest fails in symbolic/pynac.pyx
. This ticket should make sure that this does not happen again.
Change History (22)
comment:1 follow-up: ↓ 2 Changed 6 years ago by
- Cc kcrisman added
- Priority changed from major to minor
comment:2 in reply to: ↑ 1 Changed 6 years ago by
Replying to kcrisman:
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.
This is not really difficult.
The central database is a dictionary globally defined in symbolic/function.pyx
named sfunction_serial_dict
. It has pairs with key of type unsigned
and values of Function
objects (which can be GinacFunctions
or BuiltinFunctions
). Accordingly this database is filled with all known Pynac function
s (which have the GinacFunction
façade) first and then those defined via BuiltinFunction
. The unsigned
key is the serial number which, on each new function registration, is incremented by one, namely in GiNaC::function::register_new()
(https://github.com/pynac/pynac/blob/master/ginac/function.cpp#L1445) by just adding to the Pynac registry and reporting its size minus one. After the GiNaC functions have been added, the global GINAC_FN_SERIAL
is set which is then accessed via get_ginac_serial()
to get the start key of the Sage defined functions.
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.
So the solution is easy: instead of simply adding 100
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 g_registered_functions().size()
in symbolic/pynac.pyx
.
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
comment:3 Changed 6 years ago by
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 BuiltinFunction
or Function
.
comment:4 Changed 6 years ago by
- Branch set to u/rws/fix_symbolic_pynac_pyx_doctests
comment:5 Changed 6 years ago by
- Commit set to 13ba99da6b01b2b94ee67a50169567def261898b
- Milestone changed from sage-6.7 to sage-6.8
- Priority changed from minor to major
- Status changed from new to needs_review
No, that was an off-by-one error. Please review.
New commits:
13ba99d | 18257: fix symbolic/pynac.pyx doctests
|
comment:6 follow-up: ↓ 9 Changed 6 years ago by
Trivial - duplicate lines in test
+ 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
This patch seems reasonable. What was comment:3 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.
comment:7 Changed 6 years ago by
- Commit changed from 13ba99da6b01b2b94ee67a50169567def261898b to 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73
Branch pushed to git repo; I updated commit sha1. New commits:
52ccd47 | 18257: fix typo
|
comment:8 Changed 6 years ago by
comment:9 in reply to: ↑ 6 Changed 6 years ago by
Replying to kcrisman:
... What was comment:3 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.
It's taken care of in the doctests.
comment:10 Changed 6 years ago by
I'm not sure why but I still get
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) **********************************************************************
when I have both this ticket and #15024 together. Did I do something wrong? (Note: this is based off of 6.8.beta1, if it matters.)
comment:11 Changed 6 years ago by
comment:12 Changed 6 years ago by
Same branch does work with pynac-0.3.9 (in case you left that installed and that was the difference between us).
comment:13 Changed 6 years ago by
Your line numbers also don't match #18257.
comment:14 Changed 6 years ago by
Okay, I'll trash the branches I made locally and try again - may not be immediately, I'm at a conference.
comment:15 Changed 6 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to positive_review
I must have either not merged this one or perhaps in the wrong order or something... Anyway, all systems are go, and as expected #15024 fails without this and works with it. Great!
comment:16 Changed 6 years ago by
- Status changed from positive_review to needs_work
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.
comment:17 Changed 6 years ago by
- Commit changed from 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73 to 557badf1a587f24e500dd12196b14bf59a144dd1
Branch pushed to git repo; I updated commit sha1. New commits:
557badf | adjust doctest because of upstream changes
|
comment:18 Changed 6 years ago by
- Status changed from needs_work to needs_review
Could you please have a last quick look?
comment:19 Changed 6 years ago by
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).
comment:20 Changed 6 years ago by
- Branch changed from u/rws/fix_symbolic_pynac_pyx_doctests to u/rws/18257
comment:21 Changed 6 years ago by
- Commit changed from 557badf1a587f24e500dd12196b14bf59a144dd1 to 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73
- Status changed from needs_review to positive_review
comment:22 Changed 6 years ago by
- Branch changed from u/rws/18257 to 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73
- Resolution set to fixed
- Status changed from positive_review to closed
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.