Opened 4 years ago

Closed 4 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) 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: Changed 4 years ago by kcrisman

  • Cc kcrisman added
  • Priority changed from major to minor

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.

comment:2 in reply to: ↑ 1 Changed 4 years ago by rws

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 functions (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 4 years ago by rws

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 4 years ago by rws

  • Branch set to u/rws/fix_symbolic_pynac_pyx_doctests

comment:5 Changed 4 years ago by rws

  • 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:

13ba99d18257: fix symbolic/pynac.pyx doctests

comment:6 follow-up: Changed 4 years ago by kcrisman

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 4 years ago by git

  • Commit changed from 13ba99da6b01b2b94ee67a50169567def261898b to 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73

Branch pushed to git repo; I updated commit sha1. New commits:

52ccd4718257: fix typo

comment:8 Changed 4 years ago by rws

  • Authors set to Ralf Stephan

comment:9 in reply to: ↑ 6 Changed 4 years ago by rws

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 4 years ago by kcrisman

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

Last edited 4 years ago by kcrisman (previous) (diff)

comment:11 Changed 4 years ago by rws

Cannot confirm in a first attempt (I branched from #18257, merged #15024 with git trac pull, then merged develop).

comment:12 Changed 4 years ago by rws

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 4 years ago by rws

Your line numbers also don't match #18257.

comment:14 Changed 4 years ago by kcrisman

Okay, I'll trash the branches I made locally and try again - may not be immediately, I'm at a conference.

comment:15 Changed 4 years ago by kcrisman

  • 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 4 years ago by rws

  • 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 4 years ago by git

  • Commit changed from 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73 to 557badf1a587f24e500dd12196b14bf59a144dd1

Branch pushed to git repo; I updated commit sha1. New commits:

557badfadjust doctest because of upstream changes

comment:18 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

Could you please have a last quick look?

comment:19 Changed 4 years ago by kcrisman

I don't understand why this fails, though? (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).

Version 0, edited 4 years ago by kcrisman (next)

comment:20 Changed 4 years ago by rws

  • Branch changed from u/rws/fix_symbolic_pynac_pyx_doctests to u/rws/18257

comment:21 Changed 4 years ago by rws

  • Commit changed from 557badf1a587f24e500dd12196b14bf59a144dd1 to 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73
  • Status changed from needs_review to positive_review

comment:22 Changed 4 years ago by vbraun

  • Branch changed from u/rws/18257 to 52ccd47dee6b0cb6da2ad6f3855942c84a36ac73
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.