Opened 12 years ago

Closed 12 years ago

# sage-4.4.3, 4.4.4: Basic pickling bug in finite fields

Reported by: Owned by: William Stein Alex Ghitza major sage-4.6 basic arithmetic John Cremona sage-4.6.alpha1 William Stein John Cremona N/A

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

```

### comment:1 Changed 12 years ago by William Stein

Description: modified (diff) Basic pickling bug in finite fields → sage-4.4.3, 4.4.4: Basic pickling bug in finite fields

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:

```    def __reduce__(self):
"""
Used in pickling.

EXAMPLES::

sage: A = FiniteField(127)
sage: A == loads(dumps(A)) # indirect doctest
True
sage: B = FiniteField(3^3,'b')
True
sage: C = FiniteField(2^16,'c')
True
sage: D = FiniteField(3^20,'d')
True
"""
return self._factory_data[0].reduce_data(self)
```

However, _factory_data is not defined anywhere else in the source code:

```wstein@redhawk:~/build/sage-4.4.4.alpha1/devel/sage/sage/rings/finite_rings\$ grep _factory_data *.pyx *.pxd
*.py
finite_field_base.pyx:        return self._factory_data[0].reduce_data(self)
```

### comment:2 Changed 12 years ago by William Stein

Status: new → needs_review

### comment:4 Changed 12 years ago by John Cremona

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:
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>
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:
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>
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 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 John Cremona

And a similar failure in sage/rings/polynomial/multi_polynomial_libsingular.pyx

### Changed 12 years ago by William Stein

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 William Stein

Status: needs_work → needs_review

### comment:8 follow-up:  10 Changed 12 years ago by John Cremona

Authors: → William Stein → 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:9 Changed 12 years ago by John Cremona

#9409 may well be ok after this; I will test that.

### comment:10 in reply to:  8 ; follow-up:  11 Changed 12 years ago by William Stein

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 in reply to:  10 Changed 12 years ago by John Cremona

Status: needs_review → positive_review

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 William Stein

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 in reply to:  12 Changed 12 years ago by John Cremona

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 Mitesh Patel

Merged in: → sage-4.6.alpha1 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.