Opened 7 years ago

Closed 7 years ago

#13446 closed enhancement (fixed)

Revert fix (but not doctests) from #11919

Reported by: mjo Owned by: burcin
Priority: major Milestone: sage-5.6
Component: symbolics Keywords:
Cc: Merged in: sage-5.6.beta0
Authors: Michael Orlitzky Reviewers: Burcin Erocal, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by burcin)

In #11919, I added a workaround for picking of symbolic functions. An upgrade to Pynac in #12950 made the workaround unnecessary, so we should just remove it.

Apply sage-trac_13446-original_comment.patch

Attachments (1)

sage-trac_13446-original_comment.patch (713 bytes) - added by mjo 7 years ago.
Remove the fix, but leave the original comment.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by mjo

  • Authors set to Michael Orlitzky
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by burcin

Do you really need to change the text before the doctest? IMHO, we don't need to discuss the history of the bug in the reference manual.

comment:3 Changed 7 years ago by mjo

Is it a big deal? We at least need the code explaining why the doctest is there, but the explanation that's there now isn't really correct anymore. It's only a few lines and it serves to give anyone who would modify that function a little background on it.

Plus, that function doesn't wind up in the reference manual anyway, so it doesn't hurt anyone who isn't digging into the code.

comment:4 Changed 7 years ago by kcrisman

I'm somewhat surprised to find myself on the side of less historical explanation, but I agree that this is a little confusing when reading the code directly. If anything, I'd remove text so that one just tests whether pickling works - which, to be honest, we do quite a bit of in Sage, so it shouldn't need any explanation. But this is probably all a bit of bikeshedding...

Changed 7 years ago by mjo

Remove the fix, but leave the original comment.

comment:5 Changed 7 years ago by mjo

Ok, the second patch leaves the comment alone. The reviewer can thus choose his favorite version =)

comment:6 Changed 7 years ago by burcin

  • Description modified (diff)
  • Reviewers set to Burcin Erocal, Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Positive review to second version.

Apply sage-trac_13446-original_comment.patch

comment:7 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.