#6094 closed defect (fixed)
Change all occurrences of "method" to "algorithm"
Reported by: | rlm | Owned by: | rlm |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.1 |
Component: | coding theory | Keywords: | |
Cc: | wdj | Merged in: | sage-4.6.1.alpha3 |
Authors: | David Joyner, William Stein, Johan Sebastian Rosenkilde Nielsen | Reviewers: | Rob Beezer, Robert Miller |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
In a discussion with David, we realized that he's been using "method" for "algorithm" in several places. This fix will follow up on #5701, and will probably depend on the patches there.
Attachments (6)
Change History (39)
comment:1 follow-up: ↓ 2 Changed 13 years ago by
comment:2 in reply to: ↑ 1 Changed 13 years ago by
Replying to jason:
"method" is not only easier to say, but it's easier to spell, and probably is more memorable to people...
It is also *not* the standard arg name. If you think method is better, take it to the mailing list.
comment:3 follow-up: ↓ 4 Changed 13 years ago by
okay, I didn't realize there was a standard. The weight of history changes the situation a bit.
comment:4 in reply to: ↑ 3 Changed 13 years ago by
Replying to jason:
okay, I didn't realize there was a standard...
Actually, neither did I. But when David and I did a search for "method=", most of what we found were places where he was using that name.
In fact, here is a complete (I think) list of the functions which use this (4.0.alpha0):
calculus/desolvers.py: eulers_method, eulers_method_2x2 coding/decoder.py: decode coding/linear_code.py: decode, is_permutation_equivalent, permutation_automorphism_group, spectrum finance/easter.py: easter combinat/designs/block_design.py: ProjectiveGeometryDesign combinat/designs/covering_design.py: __init__, method combinat/designs/incidence_structures.py: dual_incidence_structure groups/matrix_gps/matrix_group.py: as_permutation_group, module_composition_factors schemes/elliptic_curves/padic_lseries.py: frobenius matrix/matrix_double_dense.pyx: exp matrix/matrix_modn_sparse.pyx: _rank_linbox rings/polynomial/polynomial_compiled.pyx: __init__
comment:5 Changed 13 years ago by
Thanks. I'll get to these when #5701 is applied unless someone says I should just create a patch based on the patches there.
comment:6 Changed 13 years ago by
The current patch passes sage -testall with guava removed and all patched on #5701 applied.
comment:7 Changed 13 years ago by
- Summary changed from Change all occurrences of "method" to "algorithm" to [with patch, needs review] Change all occurrences of "method" to "algorithm"
comment:8 Changed 13 years ago by
- Summary changed from [with patch, needs review] Change all occurrences of "method" to "algorithm" to [with patch, needs rebase] Change all occurrences of "method" to "algorithm"
Fails to apply cleanly to Sage 4.0.1.
applying /home/palmieri/trac_6094-method-vs-algorithm.patch patching file sage/coding/linear_code.py Hunk #1 FAILED at 315 Hunk #2 FAILED at 341 Hunk #3 FAILED at 351 Hunk #5 FAILED at 1144 Hunk #6 FAILED at 1165 Hunk #11 FAILED at 1636 Hunk #12 FAILED at 1651 Hunk #13 FAILED at 1668 Hunk #15 FAILED at 1758 Hunk #22 FAILED at 2179 10 out of 23 hunks FAILED -- saving rejects to file sage/coding/linear_code.py.rej patching file sage/combinat/designs/block_design.py Hunk #2 FAILED at 89 1 out of 3 hunks FAILED -- saving rejects to file sage/combinat/designs/block_design.py.rej abort: patch failed to apply
comment:9 Changed 13 years ago by
I don't know how to rebase. Can someone point me to a reference? I might be able to do it this weekend.
comment:10 Changed 12 years ago by
- Report Upstream set to N/A
- Status changed from needs_work to needs_review
comment:11 Changed 12 years ago by
- Reviewers set to Rob Beezer
- Status changed from needs_review to positive_review
Passes all tests. There are more instances of the use of method=
in coding/code_bounds.py which will have a patch at #7971. Positive review.
comment:12 Changed 12 years ago by
- Summary changed from [with patch, needs rebase] Change all occurrences of "method" to "algorithm" to Change all occurrences of "method" to "algorithm"
comment:13 Changed 12 years ago by
It was pointed out by people in our status reports session that this patch violates our deprecation policy. Yuck. I.e., technically we should do
def f(algorithm='default', method=None): if method is not None: deprecation('...') algorithm = method
basically *everywhere*, then remove them all in a year. Hmmmm....
comment:14 Changed 12 years ago by
- Status changed from positive_review to needs_work
I've sent this back to "needs work" to await deprecation warnings. With more to do, include the changes in ccoding/code_bounds.py here as part of this ticket (see #7971).
comment:15 follow-up: ↓ 16 Changed 12 years ago by
I wonder if it could be done with a decorator?
@deprecate_method def f(..., method="foo"): ...
comment:16 in reply to: ↑ 15 Changed 12 years ago by
Changed 12 years ago by
Rebased for 4.5.1. Change to more files and at more places as well as documentation. Added deprecation warnings.
comment:17 Changed 12 years ago by
- Status changed from needs_work to needs_review
I wrote a new patch which uses the decorator mentioned above. This patch assumes applying the patch for Trac #9907'''
I basically rewrote the patch, so it works for Sage 4.5.3 (accidentally wrote 4.5.1 in the patch message).
I also did a grep on all Sage source which uncovered more places where method should be replaced with algorithm (I assume). I also changed the doc-texts to refer to "algorithms" instead of "methods" for the relevant functions changed.
Hope I can get a fast review on this; I plan to do major relocations to the coding theory library soon, and this trac touches a lot of code in a lot of places :-)
Cheers, Johan
comment:18 Changed 12 years ago by
With #9907 applied, this applied to a 4.5.3 version with one failure (docstring change in sage/schemes/elliptic_curves/padic_lseries.py, frobenius()
, which I fixed. Also changed references to 4.5.1 to 4.5.3. Finally, the filename needs a .patch
extension to display nicely in the web page view. All in a new attachment, preserving original author's ID.
With rebase, builds just fine. Works as expected in trials with as_permutation_group()
in sage/matrix_gps/matrix_group.py
. Running full tests right now.
Minor complaint about how the decorator works, I'll put this on #9907. More here once tests finish.
comment:19 Changed 12 years ago by
- Status changed from needs_review to needs_work
Decorator complaint is bigger than these tickets. Its on sage-devel.
sage -tp 4 devel/sage yields:
sage -t devel/sage/sage/schemes/elliptic_curves/padic_lseries.py # 1 doctests failed sage -t devel/sage/sage/misc/sagedoc.py # 3 doctests failed sage -t devel/sage/sage/structure/sage_object.pyx # 1 doctests failed
which are all reproducible on my machine.
First is an easy fix, extra deprecation warning in a doctest elsewhere when using frobenius()
.
Second is just me. I nuked all my documentation while testing the new color scheme. Ignore it.
Third looks like something is pickling the two decorators and once they get moved the pickle breaks, so maybe this really belongs on #9907? Complaint follows. Parts look mysterious to me, other parts look like they involve these decorators. It could be a false alarm from my development setup.
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"
comment:20 follow-up: ↓ 21 Changed 12 years ago by
Hi Rob Great, thanks for looking into this. Sorry about the rudimentary stuff on the patch; I'm still learning the ropes.
sage_object.pyx also fails on my machine -- don't know how I missed that when testing. I'll look into that and your sage-devel thread.
Cheers, Johan
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 22 Changed 12 years ago by
Replying to jsrn:
Great, thanks for looking into this. Sorry about the rudimentary stuff on the patch; I'm still learning the ropes.
No problem. Just holler if you want to learn some new ropes.
sage_object.pyx also fails on my machine -- don't know how I missed that when testing. I'll look into that and your sage-devel thread.
If you see how to contribute to the sage-devel thread, that would be great. But don't let it delay this.
Pickling is a rat's nest. #9907 is well-intentioned, but not strictly necessary. You may not want this to depend on it? In other words, can you proceed without the move and the fix for classes? You really just want to ultimately do coding theory, right?
Rob
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 23 Changed 12 years ago by
Replying to rbeezer:
Replying to jsrn:
Great, thanks for looking into this. Sorry about the rudimentary stuff on the patch; I'm still learning the ropes.
No problem. Just holler if you want to learn some new ropes.
sage_object.pyx also fails on my machine -- don't know how I missed that when testing. I'll look into that and your sage-devel thread.
If you see how to contribute to the sage-devel thread, that would be great. But don't let it delay this.
Pickling is a rat's nest. #9907 is well-intentioned, but not strictly necessary. You may not want this to depend on it? In other words, can you proceed without the move and the fix for classes? You really just want to ultimately do coding theory, right?
Rob
I'm also interested in cleaning up and improving Sage ;-) But you're right. This opened up a can of worms, I didn't expect. I've created a new Trac #9919 for making the workaround mentioned in #9907 general by patching sage_wraps. Then I can make this patch depend only on #9919. The future of #9907 can then be decided later (the current patch there would then have to be dependent on this patch and include patching the files patched here).
comment:23 in reply to: ↑ 22 Changed 12 years ago by
Replying to jsrn:
I'm also interested in cleaning up and improving Sage ;-) But you're right. This opened up a can of worms, I didn't expect.
Of course - cleanup is definitely to be encouraged! But too many times the clean-up and cans of worms are unavoidable. When you can, best to avoid trouble and isolate the problem like you are doing.
comment:24 Changed 12 years ago by
Ok, now it gets hairy (and annoying). In plot.misc, several functions from sage.ext.fast_eval are imported; these in turn imports all sorts of stuff (e.g. rings), which means that there will be circular references from e.g. polynomial_ring when applying this patch. This just underlines the importance of moving the decorators to a general place in Trac #9907. It also means that unless e.g. this patch the one for Trac #9919 is expanded to (illogically) move the sage.ext.fast_eval import statements down into the functions in which they are used, this patch will have to depend on #9907 (which should now probably depend on #9919). Ye gods -- isolating problems are nice in theory, but...
On the upside, I am pretty sure that the problem with #9907 is easily solved -- it seems to come down to how to update the standard pickle jar. I might be wrong in this, of course.
Changed 12 years ago by
Rebased for 4.5.3. Change to more files and at more places as well as documentation. Added deprecation warnings. Fixed frobenius doctest
comment:25 Changed 12 years ago by
- Status changed from needs_work to needs_review
Ok, so now I've posted fixes for Trac #9919 and #9907. I decided to assume both of those, as it seemed hard to get around either. So, assuming both of those, the just uploaded patch gives (me) no errors. The just uploaded is like yours, Rob, but with the frobenius() doctest fixed.
As I posted on sage.devel, I also wrote a patch for the decorator-complaints in Trac #9976.
Cheers, Johan
comment:26 Changed 12 years ago by
By the way, this patch contains all changes which were at one point suggested to be contained in Trac #7971. That trac can therefore be closed/invalidated.
comment:27 Changed 12 years ago by
- Reviewers changed from Rob Beezer to Rob Beezer, Robert Miller
- Status changed from needs_review to positive_review
comment:28 Changed 12 years ago by
- Status changed from positive_review to needs_work
- Work issues set to rebase
The patch needs to be rebased to sage-4.6.1.alpha0:
patching file doc/en/constructions/linear_codes.rst patching file sage/calculus/desolvers.py Hunk #1 succeeded at 61 (offset 3 lines). Hunk #2 succeeded at 829 (offset 117 lines). Hunk #3 succeeded at 854 (offset 117 lines). Hunk #4 succeeded at 886 (offset 117 lines). Hunk #5 succeeded at 900 (offset 117 lines). Hunk #6 succeeded at 938 (offset 117 lines). Hunk #7 succeeded at 994 (offset 117 lines). patching file sage/coding/code_bounds.py patching file sage/coding/decoder.py patching file sage/coding/linear_code.py Hunk #1 succeeded at 202 (offset 2 lines). Hunk #2 succeeded at 322 (offset 2 lines). Hunk #3 succeeded at 359 (offset 2 lines). Hunk #4 succeeded at 369 (offset 2 lines). Hunk #5 succeeded at 1115 (offset 2 lines). Hunk #6 succeeded at 1143 (offset 2 lines). Hunk #7 succeeded at 1164 (offset 2 lines). Hunk #8 succeeded at 1174 (offset 2 lines). Hunk #9 succeeded at 1535 (offset 2 lines). Hunk #10 succeeded at 1556 (offset 2 lines). Hunk #11 succeeded at 1580 (offset 2 lines). Hunk #12 succeeded at 1716 (offset 2 lines). Hunk #13 succeeded at 1731 (offset 2 lines). Hunk #14 succeeded at 1745 (offset 2 lines). Hunk #15 succeeded at 1775 (offset 2 lines). Hunk #16 succeeded at 1814 (offset 2 lines). Hunk #17 succeeded at 1871 (offset 2 lines). Hunk #18 succeeded at 1891 (offset 2 lines). Hunk #19 succeeded at 1905 (offset 2 lines). Hunk #20 succeeded at 1937 (offset 2 lines). Hunk #21 succeeded at 1954 (offset 2 lines). Hunk #22 succeeded at 2289 (offset 13 lines). Hunk #23 succeeded at 2307 (offset 13 lines). Hunk #24 succeeded at 2326 (offset 13 lines). Hunk #25 succeeded at 2387 (offset 13 lines). patching file sage/combinat/designs/block_design.py patching file sage/combinat/designs/incidence_structures.py patching file sage/finance/easter.py patching file sage/groups/matrix_gps/matrix_group.py patching file sage/interfaces/ecm.py patching file sage/matrix/matrix_double_dense.pyx Hunk #4 FAILED at 1524. Hunk #5 succeeded at 1544 (offset 1 line). Hunk #6 succeeded at 1567 (offset 1 line). 1 out of 6 hunks FAILED -- saving rejects to file sage/matrix/matrix_double_dense.pyx.rej patching file sage/matrix/matrix_modn_sparse.pyx Hunk #2 succeeded at 778 with fuzz 2. Hunk #3 succeeded at 791 with fuzz 2. patching file sage/rings/polynomial/polynomial_compiled.pyx patching file sage/schemes/elliptic_curves/padic_lseries.py patching file sage/symbolic/expression.pyx Hunk #2 succeeded at 6443 (offset 745 lines). Hunk #3 succeeded at 6452 (offset 745 lines). Hunk #4 succeeded at 6504 (offset 745 lines). Hunk #5 succeeded at 6642 (offset 745 lines). Hunk #6 succeeded at 6651 (offset 745 lines). Hunk #7 succeeded at 6698 (offset 745 lines). Hunk #8 succeeded at 6718 (offset 745 lines). Hunk #9 succeeded at 6734 (offset 745 lines). Hunk #10 succeeded at 6760 (offset 745 lines). Hunk #11 succeeded at 6827 (offset 745 lines). Hunk #12 succeeded at 6864 (offset 745 lines).
Changed 12 years ago by
comment:29 Changed 12 years ago by
- Status changed from needs_work to needs_review
comment:30 Changed 12 years ago by
Robert Miller already gave the green light to the old version, so the review is only for the minor change in sage.matrix.matrix_double_dense made for rebasing.
comment:31 Changed 11 years ago by
- Status changed from needs_review to positive_review
- Work issues rebase deleted
comment:32 Changed 11 years ago by
- Merged in set to sage-4.6.1.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
"method" is not only easier to say, but it's easier to spell, and probably is more memorable to people...