Opened 12 years ago
Closed 12 years ago
#9315 closed defect (fixed)
sage-4.4.3, 4.4.4: Basic pickling bug in finite fields
Reported by: | William Stein | Owned by: | Alex Ghitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | basic arithmetic | Keywords: | |
Cc: | John Cremona | Merged in: | sage-4.6.alpha1 |
Authors: | William Stein | Reviewers: | John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
If you try to pickle *any* residue class field, then it breaks!
sage: K.<a> = NumberField(x^2 + 1) sage: F = K.factor(3)[0][0].residue_field() sage: dumps(F) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) /home/wstein/db/modsym-2010/<ipython console> in <module>() /usr/local/sage/local/lib/python2.6/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.dumps (sage/structure/sage_object.c:8367)() /usr/local/sage/local/lib/python2.6/site-packages/sage/rings/finite_rings/finite_field_base.so in sage.rings.finite_rings.finite_field_base.FiniteField.__reduce__ (sage/rings/finite_rings/finite_field_base.c:4937)() TypeError: 'NoneType' object is unsubscriptable > /home/wstein/db/modsym-2010/finite_field_base.pyx(674)sage.rings.finite_rings.finite_field_base.FiniteField.__reduce__ (sage/rings/finite_rings/finite_field_base.c:4937)()
Attachments (2)
Change History (16)
comment:1 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Summary: | Basic pickling bug in finite fields → sage-4.4.3, 4.4.4: Basic pickling bug in finite fields |
Changed 12 years ago by
Attachment: | trac_9315.patch added |
---|
comment:2 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 12 years ago by
Cc: | John Cremona added |
---|
comment:4 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
I get this doctest failure (and no more in finite_rings):
sage -t "sage/rings/finite_rings/element_ntl_gf2e.pyx" ********************************************************************** File "/home/john/sage-4.5.alpha4/devel/sage-tests/sage/rings/finite_rings/element_ntl_gf2e.pyx", line 1092: sage: f = loads(dumps(e)) Exception raised: Traceback (most recent call last): File "/home/john/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/john/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/john/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_40[4]>", line 1, in <module> f = loads(dumps(e))###line 1092: sage: f = loads(dumps(e)) File "sage_object.pyx", line 915, in sage.structure.sage_object.loads (sage/structure/sage_object.c:9175) File "element_ntl_gf2e.pyx", line 200, in sage.rings.finite_rings.element_ntl_gf2e.FiniteField_ntl_gf2e.__cinit__ (sage/rings/finite_rings/element_ntl_gf2e.cpp:3159) TypeError: __cinit__() takes at least 1 positional argument (0 given) ********************************************************************** File "/home/john/sage-4.5.alpha4/devel/sage-tests/sage/rings/finite_rings/element_ntl_gf2e.pyx", line 1093: sage: e is f Exception raised: Traceback (most recent call last): File "/home/john/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/john/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/john/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_40[5]>", line 1, in <module> e is f###line 1093: sage: e is f NameError: name 'f' is not defined ********************************************************************** File "/home/john/sage-4.5.alpha4/devel/sage-tests/sage/rings/finite_rings/element_ntl_gf2e.pyx", line 1095: sage: e == f Exception raised: Traceback (most recent call last): File "/home/john/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/john/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/john/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_40[6]>", line 1, in <module> e == f###line 1095: sage: e == f NameError: name 'f' is not defined ********************************************************************** File "/home/john/sage-4.5.alpha4/devel/sage-tests/sage/rings/finite_rings/element_ntl_gf2e.pyx", line 1449: sage: loads(dumps(a)) == a Exception raised: Traceback (most recent call last): File "/home/john/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/john/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/john/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_55[3]>", line 1, in <module> loads(dumps(a)) == a###line 1449: sage: loads(dumps(a)) == a File "sage_object.pyx", line 915, in sage.structure.sage_object.loads (sage/structure/sage_object.c:9175) File "element_ntl_gf2e.pyx", line 200, in sage.rings.finite_rings.element_ntl_gf2e.FiniteField_ntl_gf2e.__cinit__ (sage/rings/finite_rings/element_ntl_gf2e.cpp:3159) TypeError: __cinit__() takes at least 1 positional argument (0 given) ********************************************************************** File "/home/john/sage-4.5.alpha4/devel/sage-tests/sage/rings/finite_rings/element_ntl_gf2e.pyx", line 1499: sage: f = loads(dumps(e)) # indirect doctest Exception raised: Traceback (most recent call last): File "/home/john/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/john/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/john/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_57[4]>", line 1, in <module> f = loads(dumps(e)) # indirect doctest###line 1499: sage: f = loads(dumps(e)) # indirect doctest File "sage_object.pyx", line 915, in sage.structure.sage_object.loads (sage/structure/sage_object.c:9175) File "element_ntl_gf2e.pyx", line 200, in sage.rings.finite_rings.element_ntl_gf2e.FiniteField_ntl_gf2e.__cinit__ (sage/rings/finite_rings/element_ntl_gf2e.cpp:3159) TypeError: __cinit__() takes at least 1 positional argument (0 given) ********************************************************************** File "/home/john/sage-4.5.alpha4/devel/sage-tests/sage/rings/finite_rings/element_ntl_gf2e.pyx", line 1500: sage: e == f Exception raised: Traceback (most recent call last): File "/home/john/sage-current/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/john/sage-current/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/john/sage-current/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_57[5]>", line 1, in <module> e == f###line 1500: sage: e == f NameError: name 'f' is not defined ********************************************************************** 3 items had failures: 3 of 8 in __main__.example_40 1 of 4 in __main__.example_55 2 of 6 in __main__.example_57 ***Test Failed*** 6 failures. For whitespace errors, see the file /home/john/.sage//tmp/.doctest_element_ntl_gf2e.py [2.6 s] ---------------------------------------------------------------------- The following tests failed: sage -t "sage/rings/finite_rings/element_ntl_gf2e.pyx" Total time for all tests: 2.6 seconds
comment:5 Changed 12 years ago by
And a similar failure in sage/rings/polynomial/multi_polynomial_libsingular.pyx
comment:6 Changed 12 years ago by
Doctesting the whole the tree: http://sage.math.washington.edu/home/wstein/build/sage-4.5.alpha4/doctest-9315.out
Changed 12 years ago by
Attachment: | trac_9315.2.patch added |
---|
apply only this; I updated the patch to address the issues John pointed out, and clearly document what the whole point of _factory_data is (I was confused before).
comment:7 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
comment:8 follow-up: 10 Changed 12 years ago by
Authors: | → William Stein |
---|---|
Reviewers: | → John Cremona |
I am reviewing this now. Question: is it not possible to avoid the code duplication? Could we not have a __reduce()__
function in the base class that could somehow detect whether or not that is appropriate to use? Perhaps all finite fields should on creation be given a tag to say whether or not they fall under the factory framework?
I guess that you considered this already, and that it is harder than I am suggesting.
On with the testing, anyway!
comment:10 follow-up: 11 Changed 12 years ago by
Replying to cremona:
I am reviewing this now. Question: is it not possible to avoid the code duplication? Could we not have a
__reduce()__
function in the base class that could somehow detect whether or not that is appropriate to use? Perhaps all finite fields should on creation be given a tag to say whether or not they fall under the factory framework?I guess that you considered this already, and that it is harder than I am suggesting.
Yes, i tried for several hours to figure out how to do that and failed completely. Obviously, if somrbody can find a way to do whatvyou propose that might be good. But the fact is the currrent patch fixes a major bug, and nobody has suggested a better fix.
On with the testing, anyway!
comment:11 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Replying to was:
Replying to cremona:
I am reviewing this now. Question: is it not possible to avoid the code duplication? Could we not have a
__reduce()__
function in the base class that could somehow detect whether or not that is appropriate to use? Perhaps all finite fields should on creation be given a tag to say whether or not they fall under the factory framework?I guess that you considered this already, and that it is harder than I am suggesting.
Yes, i tried for several hours to figure out how to do that and failed completely. Obviously, if somrbody can find a way to do whatvyou propose that might be good. But the fact is the currrent patch fixes a major bug, and nobody has suggested a better fix.
I guessed you would have tried.
On with the testing, anyway!
All tests pass (sage -tp 10 -long).
Unfortunately, this does not fix #9409. But as it is not certain that pickling of residue fields is the issue there (though I reckon it must be) I will not delay this one on that account.
comment:12 follow-up: 13 Changed 12 years ago by
Unfortunately, this does not fix #9409. But as it is not certain
that pickling of residue fields is the issue there (though
I reckon it must be) I will not delay this one on that account.
By "pickling" maybe you mean "caching"? I didn't make any changes at all, whatsoever to caching. The only actual change my patch makes is that the default reduce method is now used when explicitly pickling residue fields. Before pickling residue fields just resulted in a big error. So what I do can't possibly fix any bug that wasn't very explicit before (i.e., "pickling doesn't work at all").
comment:13 Changed 12 years ago by
Replying to was:
Unfortunately, this does not fix #9409. But as it is not certain
that pickling of residue fields is the issue there (though
I reckon it must be) I will not delay this one on that account.
By "pickling" maybe you mean "caching"? I didn't make any changes at all, whatsoever to caching. The only actual change my patch makes is that the default reduce method is now used when explicitly pickling residue fields. Before pickling residue fields just resulted in a big error. So what I do can't possibly fix any bug that wasn't very explicit before (i.e., "pickling doesn't work at all").
OK, I just don't know what I am talking about....
comment:14 Changed 12 years ago by
Merged in: | → sage-4.6.alpha1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
There is a function in finite_field_base.pyx that tries to pickle. It has doctests but clearly can *never* actually work if run, i.e., it is never actually tested. Here's the function:
However, _factory_data is not defined anywhere else in the source code: