Opened 10 years ago

Closed 10 years ago

#9907 closed enhancement (fixed)

Move generally usable decorators to misc.decorators

Reported by: jsrn Owned by: jsrn
Priority: minor Milestone: sage-4.6.1
Component: relocation Keywords: generality, decorators
Cc: jason, mhansen Merged in: sage-4.6.1.alpha2
Authors: Johan Sebastian Rosenkilde Nielsen Reviewers: Robert Miller
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jsrn)

The decorators in sage.misc.misc should be moved to sage.misc.decorators.

In plot.misc there are also some generally usable decorators for various nice stuff. These should be moved to a general library so other modules can use them without illogically depending on plot. Simultaneously, they should be patched to use sage_wraps instead of the Python "wraps" to accommodate for Sage-specific things; e.g. the fix of Trac #9917.

Attachments (2)

trac_9907_move_decorators.patch (51.5 KB) - added by jason 10 years ago.
apply only this patch; rebased to 4.6.alpha1+some patches
trac_9907_move_decorators.2.patch (51.4 KB) - added by jsrn 10 years ago.
Rebased to 4.6. Still requires first applying patch for Trac #9919

Download all attachments as: .zip

Change History (27)

comment:1 Changed 10 years ago by jsrn

  • Status changed from new to needs_review

comment:2 follow-up: Changed 10 years ago by jsrn

While trying to use this patch in Trac #6094, I found out that the @wraps decorator used internally in all three decorators moved to sage.misc only works for functions in Python versions older than 3.2. It was fixed as Python bug issue 3445. Until Sage upgrades to such a new version of Python, I have written a small work-around, which essentially emulates the patched behaviour of wraps. This is in the newest patch.

comment:3 Changed 10 years ago by rbeezer

  • Cc jason mhansen added

I'm not qualified to check out the "work-around" for classes. But I've cc'ed Mike Hansen and Jason Grout, who I think were in on these in the first place back at #4201.

I am running this through full tests as part of #6094 and a report will go there once concluded.

Commit message on the ticket itself should probably change - there is no "as before" here to compare with, and in the final log it really won't make any sense.

comment:4 in reply to: ↑ 2 ; follow-up: Changed 10 years ago by jason

Replying to jsrn:

While trying to use this patch in Trac #6094, I found out that the @wraps decorator used internally in all three decorators moved to sage.misc only works for functions in Python versions older than 3.2. It was fixed as Python bug issue 3445. Until Sage upgrades to such a new version of Python, I have written a small work-around, which essentially emulates the patched behaviour of wraps. This is in the newest patch.

Do you think you could just add the work-around to the sage_wraps decorator (sage.misc.misc.sage_wraps) and convert the decorators to use it? There are other functions that use sage_wraps, and it would be nice to have all of that work-around code in one place.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 10 years ago by jsrn

Replying to jason:

Replying to jsrn:

While trying to use this patch in Trac #6094, I found out that the @wraps decorator used internally in all three decorators moved to sage.misc only works for functions in Python versions older than 3.2. It was fixed as Python bug issue 3445. Until Sage upgrades to such a new version of Python, I have written a small work-around, which essentially emulates the patched behaviour of wraps. This is in the newest patch.

Do you think you could just add the work-around to the sage_wraps decorator (sage.misc.misc.sage_wraps) and convert the decorators to use it? There are other functions that use sage_wraps, and it would be nice to have all of that work-around code in one place.

That should be no problem, I guess; I wasn't aware of sage_wraps. However, that might affect a lot of places outside the original scope of this ticket; should it be a new ticket then? What's the precedence here?

comment:6 in reply to: ↑ 5 Changed 10 years ago by jsrn

Replying to jsrn:

Replying to jason:

Replying to jsrn:

While trying to use this patch in Trac #6094, I found out that the @wraps decorator used internally in all three decorators moved to sage.misc only works for functions in Python versions older than 3.2. It was fixed as Python bug issue 3445. Until Sage upgrades to such a new version of Python, I have written a small work-around, which essentially emulates the patched behaviour of wraps. This is in the newest patch.

Do you think you could just add the work-around to the sage_wraps decorator (sage.misc.misc.sage_wraps) and convert the decorators to use it? There are other functions that use sage_wraps, and it would be nice to have all of that work-around code in one place.

That should be no problem, I guess; I wasn't aware of sage_wraps. However, that might affect a lot of places outside the original scope of this ticket; should it be a new ticket then? What's the precedence here?

I started Trac #9919 for doing this patch in sage_wraps. I'm also thinking that, when the problems with this patch are identified, we might move sage_wraps and other decorators in sage.misc.misc into sage.misc.decorators as well.

comment:7 Changed 10 years ago by jason

Starting another ticket sounds good.

comment:8 Changed 10 years ago by jsrn

  • Status changed from needs_review to needs_work

Rob Beezer posted the following problem with the current patch for Trac #6094, but that is actually a problem with the current patch for this trac. When testing the unpickle-function in sage.structure.sage_object, the relocation of the decorators yield problems:

rob@wave:/sage/dev$ ./sage -t  devel/sage/sage/structure/sage_object.pyx
sage -t  "devel/sage/sage/structure/sage_object.pyx"        
**********************************************************************
File "/sage/dev/devel/sage/sage/structure/sage_object.pyx", line 1001:
    sage: print "x"; sage.structure.sage_object.unpickle_all(std)
Expected:
    x...
    Successfully unpickled ... objects.
    Failed to unpickle 0 objects.
Got:
    x
    doctest:1: DeprecationWarning: Your word object is saved in an old file format since FiniteWord_over_OrderedAlphabet is deprecated and will be deleted in a future version of Sage (you can use FiniteWord_list instead). You can re-save your word by typing "word.save(filename)" to ensure that it will load in future versions of Sage.
    doctest:1: DeprecationWarning: Your word object is saved in an old file format since AbstractWord is deprecated and will be deleted in a future version of Sage (you can use FiniteWord_list instead). You can re-save your word by typing "word.save(filename)" to ensure that it will load in future versions of Sage.
    doctest:1: DeprecationWarning: Your word object is saved in an old file format since Word_over_Alphabet is deprecated and will be deleted in a future version of Sage (you can use FiniteWord_list instead). You can re-save your word by typing "word.save(filename)" to ensure that it will load in future versions of Sage.
    doctest:1: DeprecationWarning: Your word object is saved in an old file format since Word_over_OrderedAlphabet is deprecated and will be deleted in a future version of Sage (you can use FiniteWord_list instead). You can re-save your word by typing "word.save(filename)" to ensure that it will load in future versions of Sage.
    doctest:1: DeprecationWarning: ChristoffelWord_Lower is deprecated, use LowerChristoffelWord instead
    ** failed:  _class__sage_plot_misc_options__.sobj
    ** failed:  _class__sage_plot_misc_rename_keyword__.sobj
    Failed:
    _class__sage_plot_misc_options__.sobj
    _class__sage_plot_misc_rename_keyword__.sobj
    Successfully unpickled 584 objects.
    Failed to unpickle 2 objects.
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_23
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/rob/.sage//tmp/.doctest_sage_object.py
         [5.2 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t  "devel/sage/sage/structure/sage_object.pyx"

The deprecation-warnings are just garbage from words.py, so the important thing to note is the " failed"-lines. The problem, as far as I can see, is simply that there is a standard pickle jar which has a pickle for all the functions etc. shipped with sage. This needs to be updated when applying this patch. Does anyone know how this is done and possibly included in the patch?

Cheers, Johan

comment:9 follow-up: Changed 10 years ago by mhansen

The pickle jar contains old pickles of objects that we want to ensure work in the future. For example, if you made the changes at 9907 as the are, then possibly many pickles out in the wild would break. Instead of trying to update the pickle jar, the appropriate thing to do would be to add the following to sage/plot/misc.py

#For backward compatibility -- see #9907.
from sage.misc.decorators import options, suboptions, rename_keyword

That way, the old pickles will still work as they will still be able to find the decorators in sage.plot.misc.

comment:10 in reply to: ↑ 9 Changed 10 years ago by jsrn

Replying to mhansen:

The pickle jar contains old pickles of objects that we want to ensure work in the future. For example, if you made the changes at 9907 as the are, then possibly many pickles out in the wild would break. Instead of trying to update the pickle jar, the appropriate thing to do would be to add the following to sage/plot/misc.py

#For backward compatibility -- see #9907.
from sage.misc.decorators import options, suboptions, rename_keyword

That way, the old pickles will still work as they will still be able to find the decorators in sage.plot.misc.

Ok, thanks -- I will update the patch for #9907. I was actually wondering a bit about backward compatibility, so good to know this is the system. Does the above fix then have a one-year lifetime like for keyword-deprecation? If so, should it be flagged somehow (e.g. with the comment containing the term "backward compatibility") so it can be found and removed one year from now?

comment:11 Changed 10 years ago by jsrn

  • Description modified (diff)
  • Summary changed from Move generally usable decorators from plot.misc to misc.decorators to Move generally usable decorators to misc.decorators

comment:12 Changed 10 years ago by jsrn

  • Description modified (diff)

comment:13 Changed 10 years ago by jsrn

  • Status changed from needs_work to needs_review

comment:14 Changed 10 years ago by jsrn

  • Owner changed from tdb to jsrn

Note that the added patch assumes the patch for Trac #9919.

comment:15 Changed 10 years ago by jsrn

Added a comment about when then bug in #9919 was fixed in Python. Accidentally uploaded two.

comment:16 Changed 10 years ago by jsrn

Ok, uploaded a new one, as it seems you can't manually change the patch-files because of some hashes in the top of the file. So this one was done the right way. Remember to use the newest and NOT the one called ".2.patch".

comment:17 follow-up: Changed 10 years ago by jason

Can I delete the .2.patch file, then?

comment:18 Changed 10 years ago by jason

I've opened up #10057 to address an import change that needs to be made in sagenb after this is merged.

Changed 10 years ago by jason

apply only this patch; rebased to 4.6.alpha1+some patches

comment:19 in reply to: ↑ 17 Changed 10 years ago by jsrn

Replying to jason:

Can I delete the .2.patch file, then?

Sure; I just don't have the privileges to do that myself.

comment:20 Changed 10 years ago by mhansen

This needs to be coordinated with #10107.

Changed 10 years ago by jsrn

Rebased to 4.6. Still requires first applying patch for Trac #9919

comment:21 Changed 10 years ago by rlm

  • Reviewers set to Robert Miller
  • Status changed from needs_review to positive_review

If I apply #6094, #9907, #9919, and #10107 together on top of sage-4.6, all long tests pass. The code looks good.

comment:22 Changed 10 years ago by jdemeyer

Should this be merged in Sage 4.6.1? (if yes, please set the Milestone)

comment:23 Changed 10 years ago by jsrn

  • Milestone set to sage-4.6.1

comment:24 Changed 10 years ago by jdemeyer

  • Authors changed from jsrn to Johan Sebastian Rosenkilde Nielsen

comment:25 Changed 10 years ago by jdemeyer

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