Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#11919 closed defect (fixed)

Issue when pickling a formal function

Reported by: cdsousa Owned by: burcin
Priority: minor Milestone: sage-5.1
Component: symbolics Keywords: pynac, pickling
Cc: jpflori, burcin, jason, dsm, mjo Merged in: sage-5.1.beta1
Authors: Michael Orlitzky Reviewers: Nils Bruin, Burcin Erocal
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

There is a problem in symbolic function pickling:

sage: x = var('x'); f = function('f',x) ; s = dumps(f) ; loads(s)
RuntimeError: unknown function 'f' in archive

The error was not present in Sage 4.7 but it is in newer versions. It was suggested (http://groups.google.com/group/sage-support/browse_thread/thread/b439844f2fa0b675) that it is related to Pynac. Pynac was updated to 0.2.2 (sage-4.7.1.alpha1) and then to 0.2.3 (sage-4.7.1.alpha4) according to sage 4.7.1 changelog.

Attachments (1)

sage-trac_11919.patch (1.2 KB) - added by mjo 8 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:2 Changed 8 years ago by nbruin

  • Cc jpflori burcin added
  • Component changed from pickling to symbolics
  • Keywords pynac pickling added
  • Owner changed from was to burcin

comment:3 Changed 8 years ago by jason

  • Cc jason added

comment:4 Changed 8 years ago by dsm

  • Cc dsm added

comment:5 Changed 8 years ago by mjo

  • Cc mjo added

I hit this independently using deepcopy(). In sage-4.7:

sage: f = function('f',x)
sage: deepcopy(f)
f(x)

and in 4.8.alpha5:

sage: f = function('f',x)
sage: deepcopy(f)
...
RuntimeError: unknown function 'f' in archive

comment:6 Changed 8 years ago by mjo

Interestingly, if I take the pynac spkg from 4.7 and install it on 5.0.beta7, it doesn't fix the crash.

comment:7 Changed 8 years ago by mjo

I built a copy of 4.7.1, and the crash does happen. So the change was introduced between 4.7 and 4.7.1.

comment:8 follow-up: Changed 8 years ago by mjo

Okay, this broke with #9240. I can fix it with a one-line change in 4.7.1.alpha4:

diff --git a/sage/symbolic/function.pyx b/sage/symbolic/function.pyx
--- a/sage/symbolic/function.pyx
+++ b/sage/symbolic/function.pyx
@@ -127,6 +127,7 @@
     cdef _register_function(self):
         cdef GFunctionOpt opt
         opt = g_function_options_args(self._name, self._nargs)
+        opt.set_python_func()

         if hasattr(self, '_eval_'):
             opt.eval_func(self)

But there's no longer a set_python_func() method on GFunctionOpt, so I don't know how to fix it in 5.0.beta7. The adventure continues...

comment:9 in reply to: ↑ 8 Changed 8 years ago by mjo

Replying to mjo:

But there's no longer a set_python_func() method on GFunctionOpt, so I don't know how to fix it in 5.0.beta7. The adventure continues...

Disregard that, I'm a moron. Testing a patch now.

Changed 8 years ago by mjo

comment:10 Changed 8 years ago by mjo

  • Authors set to Michael Orlitzky
  • Status changed from new to needs_review

comment:11 follow-up: Changed 8 years ago by abid_naqvi83

I had the same problem on Sage 4.7.1. I applied the patch described above but it didn't work (same error message). I then uninstalled sage and installed version 4.8 instead. I applied the patch again and it still didn't work. Any suggestions?

comment:12 in reply to: ↑ 11 Changed 8 years ago by mjo

Replying to abid_naqvi83:

I had the same problem on Sage 4.7.1. I applied the patch described above but it didn't work (same error message). I then uninstalled sage and installed version 4.8 instead. I applied the patch again and it still didn't work. Any suggestions?

Did you remember to run sage -b after applying the patch?

I don't remember which version I created the patch against... Does it at least apply cleanly against 4.8?

comment:13 follow-up: Changed 8 years ago by nbruin

  • Status changed from needs_review to positive_review

Fix confirmed! indeed, the patch simply adds back a line that disappeared in #9240 without any reason stated, so probably that was just an accident.

Positive review!

(for abid_naqvi83: note that the patchbot is happy and that the feature is actually doctested. If the fix has no effect, perhaps you forgot to run "sage -b"?)

comment:14 Changed 8 years ago by abid_naqvi83

That was it. Didn't know about "sage -b". Sorry for the faux pas and thanks for the fix. Working now.

comment:15 in reply to: ↑ 13 Changed 8 years ago by burcin

  • Status changed from positive_review to needs_work

Replying to nbruin:

Fix confirmed! indeed, the patch simply adds back a line that disappeared in #9240 without any reason stated, so probably that was just an accident.

It was not an accident. The python_func flag stored in pynac is not a bool any more. It is a bitmask that marks which custom functions are implemented in Python. The rest, if they exist are C++ functions.

Positive review!

This patch doesn't fix the problem. It might actually lead to crashes, since pynac will look for a python function to call for evaluation, differentiation, etc. if some bits in python_func are set.

The correct fix will be to get pynac to create a new dummy symbolic function at the point where it raises an error with "unknown function 'f' in archive." Relevant code can be found in ginac/function.{h,cpp} in pynac sources.

comment:16 Changed 8 years ago by burcin

  • Reviewers set to Nils Bruin, Burcin Erocal
  • Status changed from needs_work to positive_review

I take it back. This is a good workaround, since constructors of function_options takes care to clear the function pointers used to store references to the custom methods which can be defined for evalutation, conjugation, etc. Before trying to evaluate any of these custom methods, we check if the pointer is NULL, so a crash can never happen.

comment:17 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-5.1

comment:18 follow-up: Changed 8 years ago by burcin

Pynac 0.2.4 from #12950 fixes this, without the patch attached to this ticket. I don't know how long it will take to get that reviewed and merged, so I'm not opposed to merging this first.

comment:19 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.1.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by mjo

Replying to burcin:

Pynac 0.2.4 from #12950 fixes this, without the patch attached to this ticket. I don't know how long it will take to get that reviewed and merged, so I'm not opposed to merging this first.

Now that #12950 is in, should we remove the line again (but keep the test)?

comment:21 in reply to: ↑ 20 Changed 7 years ago by burcin

Replying to mjo:

Replying to burcin:

Pynac 0.2.4 from #12950 fixes this, without the patch attached to this ticket. I don't know how long it will take to get that reviewed and merged, so I'm not opposed to merging this first.

Now that #12950 is in, should we remove the line again (but keep the test)?

Yes, though it is mostly harmless AFAICT.

comment:22 Changed 7 years ago by mjo

I've removed it at #13446 (needs review if anyone wants it).

Note: See TracTickets for help on using tickets.