Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#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:

Status badges

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)

trac_6094-method-vs-algorithm.patch (41.5 KB) - added by wdj 12 years ago.
based on 4.0.rc0 and all patches in #5701
trac_6094-rebased_against_4.3.1.rc0.patch (41.3 KB) - added by was 11 years ago.
I rebased this against 4.3.1.rc0.
trac_6094-4.5.1.with_deprecation (65.8 KB) - added by jsrn 11 years ago.
Rebased for 4.5.1. Change to more files and at more places as well as documentation. Added deprecation warnings.
trac_6094-4.5.3.with_deprecation.patch (65.9 KB) - added by rbeezer 11 years ago.
Rebased, renamed, commit message edited
trac_6094-4.5.3.with_deprecation.2.patch (66.7 KB) - added by jsrn 11 years ago.
Rebased for 4.5.3. Change to more files and at more places as well as documentation. Added deprecation warnings. Fixed frobenius doctest
trac_6094-4.6.1.alpha0.with_deprecation.patch (66.8 KB) - added by jsrn 10 years ago.
Rebased for 4.6.1.alpha0. Still requires patches for #9919 and #9907 (in that order)

Download all attachments as: .zip

Change History (39)

comment:1 follow-up: Changed 12 years ago by jason

"method" is not only easier to say, but it's easier to spell, and probably is more memorable to people...

comment:2 in reply to: ↑ 1 Changed 12 years ago by rlm

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: Changed 12 years ago by jason

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 12 years ago by rlm

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 12 years ago by wdj

Thanks. I'll get to these when #5701 is applied unless someone says I should just create a patch based on the patches there.

Changed 12 years ago by wdj

based on 4.0.rc0 and all patches in #5701

comment:6 Changed 12 years ago by wdj

The current patch passes sage -testall with guava removed and all patched on #5701 applied.

comment:7 Changed 12 years ago by wdj

  • 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 12 years ago by jhpalmieri

  • 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 12 years ago by wdj

I don't know how to rebase. Can someone point me to a reference? I might be able to do it this weekend.

Changed 11 years ago by was

I rebased this against 4.3.1.rc0.

comment:10 Changed 11 years ago by was

  • Report Upstream set to N/A
  • Status changed from needs_work to needs_review

comment:11 Changed 11 years ago by rbeezer

  • Authors set to David Joyner, William Stein
  • 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 11 years ago by rbeezer

  • 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 11 years ago by was

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 11 years ago by rbeezer

  • 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: Changed 11 years ago by was

I wonder if it could be done with a decorator?

@deprecate_method
def f(..., method="foo"):
    ...

comment:16 in reply to: ↑ 15 Changed 11 years ago by jason

Replying to was:

I wonder if it could be done with a decorator?

Yep. See the already-merged #8607.

Changed 11 years ago by jsrn

Rebased for 4.5.1. Change to more files and at more places as well as documentation. Added deprecation warnings.

comment:17 Changed 11 years ago by jsrn

  • Authors changed from David Joyner, William Stein to David Joyner, William Stein, Johan S. R. Nielsen
  • 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

Changed 11 years ago by rbeezer

Rebased, renamed, commit message edited

comment:18 Changed 11 years ago by rbeezer

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 11 years ago by rbeezer

  • 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: Changed 11 years ago by jsrn

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: Changed 11 years ago by 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

comment:22 in reply to: ↑ 21 ; follow-up: Changed 11 years ago by jsrn

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 11 years ago by rbeezer

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 11 years ago by jsrn

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 11 years ago by jsrn

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 11 years ago by jsrn

  • 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 11 years ago by jsrn

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 11 years ago by rlm

  • Reviewers changed from Rob Beezer to Rob Beezer, 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.

I have also closed #7971.

comment:28 Changed 11 years ago by jdemeyer

  • 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 10 years ago by jsrn

Rebased for 4.6.1.alpha0. Still requires patches for #9919 and #9907 (in that order)

comment:29 Changed 10 years ago by jsrn

  • Status changed from needs_work to needs_review

comment:30 Changed 10 years ago by jsrn

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 10 years ago by rlm

  • Status changed from needs_review to positive_review
  • Work issues rebase deleted

comment:32 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 10 years ago by jdemeyer

  • Authors changed from David Joyner, William Stein, Johan S. R. Nielsen to David Joyner, William Stein, Johan Sebastian Rosenkilde Nielsen
Note: See TracTickets for help on using tickets.