Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#27539 closed enhancement (fixed)

py3: add __reduce__ to eclib modular symbol

Reported by: chapoton Owned by: chapoton
Priority: major Milestone: sage-8.8
Component: python3 Keywords:
Cc: embray, jdemeyer, tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 600d921 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by embray)

to fix the last doctest in schemes/elliptic_curves/:

sage -t --long src/sage/schemes/elliptic_curves/padic_lseries.py
**********************************************************************
File "src/sage/schemes/elliptic_curves/padic_lseries.py", line 155, in sage.schemes.elliptic_curves.padic_lseries.pAdicLseries
Failed example:
    lp == loads(dumps(lp))
Exception raised:
    Traceback (most recent call last):
      File "sage/misc/persist.pyx", line 284, in sage.misc.persist.dumps (build/cythonized/sage/misc/persist.c:4158)
        return obj.dumps(compress)
      File "sage/structure/sage_object.pyx", line 464, in sage.structure.sage_object.SageObject.dumps (build/cythonized/sage/structure/sage_object.c:3645)
        return _base_dumps(self, compress=compress)
      File "sage/misc/persist.pyx", line 257, in sage.misc.persist._base_dumps (build/cythonized/sage/misc/persist.c:3892)
        gherkin = SagePickler.dumps(obj)
      File "sage/misc/persist.pyx", line 836, in sage.misc.persist.SagePickler.dumps (build/cythonized/sage/misc/persist.c:6607)
        pickler.dump(obj)
    TypeError: can't pickle sage.libs.eclib.newforms.ECModularSymbol objects

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.schemes.elliptic_curves.padic_lseries.pAdicLseries[18]>", line 1, in <module>
        lp == loads(dumps(lp))
      File "sage/misc/persist.pyx", line 286, in sage.misc.persist.dumps (build/cythonized/sage/misc/persist.c:4211)
        return _base_dumps(obj, compress=compress)
      File "sage/misc/persist.pyx", line 257, in sage.misc.persist._base_dumps (build/cythonized/sage/misc/persist.c:3892)
        gherkin = SagePickler.dumps(obj)
      File "sage/misc/persist.pyx", line 836, in sage.misc.persist.SagePickler.dumps (build/cythonized/sage/misc/persist.c:6607)
        pickler.dump(obj)
    TypeError: can't pickle sage.libs.eclib.newforms.ECModularSymbol objects
**********************************************************************
1 item had failures:
   1 of  20 in sage.schemes.elliptic_curves.padic_lseries.pAdicLseries
    [216 tests, 1 failure, 19.32 s]

Change History (12)

comment:1 Changed 5 months ago by chapoton

  • Branch set to u/chapoton/eclib_ms_reduce
  • Commit set to 600d921515514ee9c74eb0cd68c69ea68f72ffca
  • Status changed from new to needs_review

New commits:

600d921add __reduce__ for eclib modular symbols

comment:2 Changed 5 months ago by chapoton

  • Cc embray jdemeyer tscrim added

green bot, please review

comment:3 Changed 5 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:4 Changed 5 months ago by slelievre

  • Summary changed from py3: add _reduce__ to eclib modular symbol to py3: add __reduce__ to eclib modular symbol

comment:5 Changed 5 months ago by embray

  • Status changed from positive_review to needs_info

It might be good if any test for a __reduce__ method actually demonstrated pickling/unpickling, since that's what this is for.

A more general question though: Although I'm sure there's a good reason, it's not entirely clear from this ticket, or your commit message, why adding this was necessary for Python 3 support? What exactly is this fixing, and why did this need a special __reduce__ method when it didn't before?

comment:6 Changed 5 months ago by vbraun

  • Branch changed from u/chapoton/eclib_ms_reduce to 600d921515514ee9c74eb0cd68c69ea68f72ffca
  • Resolution set to fixed
  • Status changed from needs_info to closed

comment:7 Changed 5 months ago by vbraun

  • Commit 600d921515514ee9c74eb0cd68c69ea68f72ffca deleted

just noticed that there is more positive review necromancy on this ticket, but the question is imho better suited for sage-devel so how about you take it there?

comment:8 Changed 5 months ago by embray

Not really--I think Frederic could easily answer my question here. I don't mind that it was merged but I still think this should be answered better...

comment:9 Changed 5 months ago by embray

  • Owner changed from (none) to chapoton

comment:10 Changed 5 months ago by tscrim

So I can somewhat answer why I believe there was the necessity. The class is only a subclass of object, so it has no pickling support by default, which I think the python behavior changed in regard to how it handles this. My guess is some test was there is a test that uses this as part of its pickle.

comment:11 Changed 5 months ago by vbraun

I still think that information about general py2/3 differences in Sage code should be publicized more widely

comment:12 Changed 5 months ago by embray

  • Description modified (diff)

I ran the tests for this sub-package on an older Python 3 build and had two test failures, one of which was obviously the one that this ticket was intended to fix, so I have updated the description with that additional info.

The question you have to ask is why does this fail on Python 3 but not on Python 2? I've spent a bit of time looking into it and this is still not entirely clear to me. This may well be the simplest and most obvious solution, and I have no problem with it on its face. I'm just a little unsettled by not knowing why this worked before and now doesn't (even with the same pickle protocol it fails).

Note: See TracTickets for help on using tickets.