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 )
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)
Change History (27)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 4 Changed 10 years ago by
comment:3 Changed 10 years ago by
- 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: ↓ 5 Changed 10 years ago by
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: ↓ 6 Changed 10 years ago by
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
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
Starting another ticket sounds good.
comment:8 Changed 10 years ago by
- 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: ↓ 10 Changed 10 years ago by
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
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_keywordThat 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
- 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
- Description modified (diff)
comment:13 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 10 years ago by
- Owner changed from tdb to jsrn
Note that the added patch assumes the patch for Trac #9919.
comment:15 Changed 10 years ago by
Added a comment about when then bug in #9919 was fixed in Python. Accidentally uploaded two.
comment:16 Changed 10 years ago by
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: ↓ 19 Changed 10 years ago by
Can I delete the .2.patch file, then?
comment:18 Changed 10 years ago by
I've opened up #10057 to address an import change that needs to be made in sagenb after this is merged.
comment:19 in reply to: ↑ 17 Changed 10 years ago by
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
This needs to be coordinated with #10107.
comment:21 Changed 10 years ago by
- Reviewers set to Robert Miller
- Status changed from needs_review to positive_review
comment:22 Changed 10 years ago by
Should this be merged in Sage 4.6.1? (if yes, please set the Milestone)
comment:23 Changed 10 years ago by
- Milestone set to sage-4.6.1
comment:24 Changed 10 years ago by
comment:25 Changed 10 years ago by
- Merged in set to sage-4.6.1.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
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.