#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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 2 years ago by
- Branch set to u/chapoton/eclib_ms_reduce
- Commit set to 600d921515514ee9c74eb0cd68c69ea68f72ffca
- Status changed from new to needs_review
comment:3 Changed 2 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM.
comment:4 Changed 2 years ago by
- Summary changed from py3: add _reduce__ to eclib modular symbol to py3: add __reduce__ to eclib modular symbol
comment:5 Changed 2 years ago by
- 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 2 years ago by
- Branch changed from u/chapoton/eclib_ms_reduce to 600d921515514ee9c74eb0cd68c69ea68f72ffca
- Resolution set to fixed
- Status changed from needs_info to closed
comment:7 Changed 2 years ago by
- 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 2 years ago by
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 2 years ago by
- Owner changed from (none) to chapoton
comment:10 Changed 2 years ago by
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 2 years ago by
I still think that information about general py2/3 differences in Sage code should be publicized more widely
comment:12 Changed 2 years ago by
- 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).
New commits:
add __reduce__ for eclib modular symbols