#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 )
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)
Change History (23)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Cc jpflori burcin added
- Component changed from pickling to symbolics
- Keywords pynac pickling added
- Owner changed from was to burcin
comment:3 Changed 9 years ago by
- Cc jason added
comment:4 Changed 9 years ago by
- Cc dsm added
comment:5 Changed 9 years ago by
- Cc mjo added
comment:6 Changed 9 years ago by
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 9 years ago by
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: ↓ 9 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
comment:10 Changed 9 years ago by
- Status changed from new to needs_review
comment:11 follow-up: ↓ 12 Changed 9 years ago by
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 9 years ago by
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: ↓ 15 Changed 9 years ago by
- 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 9 years ago by
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 9 years ago by
- 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 9 years ago by
- 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 9 years ago by
- Milestone changed from sage-5.0 to sage-5.1
comment:18 follow-up: ↓ 20 Changed 9 years ago by
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 9 years ago by
- 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: ↓ 21 Changed 8 years ago by
comment:21 in reply to: ↑ 20 Changed 8 years ago by
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 8 years ago by
I've removed it at #13446 (needs review if anyone wants it).
I hit this independently using deepcopy(). In sage-4.7:
and in 4.8.alpha5: