Opened 4 years ago

Closed 4 years ago

#17973 closed enhancement (fixed)

Better Sage consistency for naming and calling in linear_code

Reported by: dlucas Owned by:
Priority: major Milestone: sage-6.6
Component: coding theory Keywords:
Cc: jsrn, defeo, ncohen Merged in:
Authors: David Lucas Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: cd3c6ee (Commits) Commit: cd3c6eea6fc2f7b4c6412728a5c8ae6a73aa14bd
Dependencies: Stopgaps:

Description

Some method and parameter names in linear_code.py file are abbreviated names which might be confusing (for instance the distance parameter actually stands for the minimum distance). Most importantly, the gen_mat method will be renamed generator_matrix and the check_mat method parity_check_matrix.

Besides, some getter methods to access the private fields of linear codes exist but are not used internally in the class. To support subclassing, it is better to use these instead of directly invoking a parameter.

Change History (14)

comment:1 Changed 4 years ago by ncohen

  • Cc ncohen added

comment:2 Changed 4 years ago by dlucas

  • Branch set to u/dlucas/better_sage_consistency_for_naming_and_calling_in_linear_code

comment:3 Changed 4 years ago by dlucas

  • Authors set to David Lucas
  • Commit set to 7b7395eae5ca1897fa9edf445c5317b1ada00584
  • Status changed from new to needs_review

New commits:

80770c5Replaced gen_mat by generator_matrix
274d72eChanged names of linear code parameters
f57ba38Class parameters are now accessed by getter methods
7b7395eReplaced check_mat method by parity_check_matrix

comment:4 Changed 4 years ago by ncohen

  • Status changed from needs_review to needs_work

Hello !

I would say that this is good to go, except for one detail: could you deprecate the methods using deprecated_function_alias instead of doing it manually? There are some advantages, like a warning in the doc of the deprecated function. Besides, they do not appear in the lists of undocumented functions.

http://www.sagemath.org/doc/developer/coding_in_python.html#deprecation

Nathann

comment:5 Changed 4 years ago by git

  • Commit changed from 7b7395eae5ca1897fa9edf445c5317b1ada00584 to 8813983b5f85ceeb0be6377d0248cc1eb19c1e6a

Branch pushed to git repo; I updated commit sha1. New commits:

8813983Replaced call to deprecation by the method deprecation_function_alias

comment:6 Changed 4 years ago by dlucas

It's done! I also replaced "check matrix" by "parity check matrix" in parity_check_matrix docstring for consistency with its new name.

comment:7 Changed 4 years ago by dlucas

  • Status changed from needs_work to needs_review

comment:8 Changed 4 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to needs_work

There are two broken doctests in binary_code.pyx:

(tmp|✔)~/sage/coding$ sage -tp 4 -l binary_code.pyx
too many failed tests, not using stored timings
Running doctests with ID 2015-03-17-17-31-39-70361bd3.
Git branch: tmp
Doctesting 1 file using 4 threads.
sage -t --long binary_code.pyx
**********************************************************************
File "binary_code.pyx", line 1100, in sage.coding.binary_code.BinaryCode.apply_permutation
Failed example:
    B = BinaryCode(codes.ExtendedBinaryGolayCode().gen_mat())
Expected nothing
Got:
    doctest:1: DeprecationWarning: gen_mat is deprecated. Please use generator_matrix instead.
    See http://trac.sagemath.org/17973 for details.
**********************************************************************
File "binary_code.pyx", line 3858, in sage.coding.binary_code.BinaryCodeClassifier.put_in_canonical_form
Failed example:
    B = BinaryCode(codes.ExtendedBinaryGolayCode().gen_mat())
Expected nothing
Got:
    doctest:1: DeprecationWarning: gen_mat is deprecated. Please use generator_matrix instead.
    See http://trac.sagemath.org/17973 for details.
**********************************************************************
2 items had failures:
   1 of   6 in sage.coding.binary_code.BinaryCode.apply_permutation
   1 of   8 in sage.coding.binary_code.BinaryCodeClassifier.put_in_canonical_form
    [351 tests, 2 failures, 6.60 s]
----------------------------------------------------------------------
sage -t --long binary_code.pyx  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 6.7 seconds
    cpu time: 6.3 seconds
    cumulative wall time: 6.6 seconds

comment:9 Changed 4 years ago by ncohen

And many others in src/doc/en/constructions/linear_codes.rst. In some situations it is safer to run all of Sage's doctests just to make sure. There may be others in places I could not guess.

Nathann

comment:10 Changed 4 years ago by git

  • Commit changed from 8813983b5f85ceeb0be6377d0248cc1eb19c1e6a to cd3c6eea6fc2f7b4c6412728a5c8ae6a73aa14bd

Branch pushed to git repo; I updated commit sha1. New commits:

7df4c04Fixed 2 broken doctests
3059874Fixed 4 broken doctests
cd3c6eeFixed 2 broken doctests

comment:11 Changed 4 years ago by dlucas

Thanks for the advice. I was indeed able to find some more after a make -ptestlong.

comment:12 Changed 4 years ago by dlucas

  • Status changed from needs_work to needs_review

comment:13 Changed 4 years ago by ncohen

  • Status changed from needs_review to positive_review

OKayyyyyyyyyyy then it's good to go !

Nathann

comment:14 Changed 4 years ago by vbraun

  • Branch changed from u/dlucas/better_sage_consistency_for_naming_and_calling_in_linear_code to cd3c6eea6fc2f7b4c6412728a5c8ae6a73aa14bd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.