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:

Status badges

Description (last modified by William Stein)

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)

trac_9315.patch (1.5 KB) - added by William Stein 12 years ago.
trac_9315.2.patch (5.2 KB) - added by William Stein 12 years ago.
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).

Download all attachments as: .zip

Change History (16)

comment:1 Changed 12 years ago by William Stein

Description: modified (diff)
Summary: Basic pickling bug in finite fieldssage-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')
            sage: B == loads(dumps(B))
            True
            sage: C = FiniteField(2^16,'c')
            sage: C == loads(dumps(C))
            True
            sage: D = FiniteField(3^20,'d')
            sage: D == loads(dumps(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)

Changed 12 years ago by William Stein

Attachment: trac_9315.patch added

comment:2 Changed 12 years ago by William Stein

Status: newneeds_review

comment:3 Changed 12 years ago by John Cremona

Cc: John Cremona added

comment:4 Changed 12 years ago by John Cremona

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

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

Changed 12 years ago by William Stein

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

Status: needs_workneeds_review

comment:8 Changed 12 years ago by John Cremona

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: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 ; Changed 12 years ago by William Stein

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

Status: needs_reviewpositive_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 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

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

Merged in: sage-4.6.alpha1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.