Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename matrix_mod2e_dense to matrix_gf2e_dense #19240

Closed
jdemeyer opened this issue Sep 18, 2015 · 23 comments
Closed

Rename matrix_mod2e_dense to matrix_gf2e_dense #19240

jdemeyer opened this issue Sep 18, 2015 · 23 comments

Comments

@jdemeyer
Copy link

The name matrix_mod2e_dense suggests that it's about matrices in ZZ/(2^e)ZZ, while it's really for matrices in GF(2^e).

Since the module and class names are non-public implementation details, there is no need for deprecation.

CC: @simon-king-jena

Component: linear algebra

Author: Jeroen Demeyer

Branch/Commit: 1e47929

Reviewer: Simon King

Issue created by migration from https://trac.sagemath.org/ticket/19240

@jdemeyer jdemeyer added this to the sage-6.9 milestone Sep 18, 2015
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

@jdemeyer
Copy link
Author

New commits:

1e6264dRename matrix_mod2e_dense to matrix_gf2e_dense

@jdemeyer
Copy link
Author

Commit: 1e6264d

@jdemeyer

This comment has been minimized.

@simon-king-jena
Copy link
Member

comment:4

Even if you think that no deprecation is needed, I do believe that it is at least needed to be able to read old pickles of matrices over GF(2^e). Users are likely to have those stored somewhere, as part of ubiquitous applications.

I don't see how you take care of that in your commit.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2015

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

fec4bf1Fix unpickling old Matrix_mod2e_dense instances

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2015

Changed commit from 1e6264d to fec4bf1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 19, 2015

Changed commit from fec4bf1 to 26f359e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 19, 2015

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

26f359eFix unpickling old Matrix_mod2e_dense instances

@simon-king-jena
Copy link
Member

comment:8

Apparently there is a malformed docstring:

Error building the documentation.
Traceback (most recent call last):
  File "/home/king/Sage/git/sage/src/doc/common/builder.py", line 1626, in <module>
    getattr(get_builder(name), type)()
  File "/home/king/Sage/git/sage/src/doc/common/builder.py", line 292, in _wrapper
    getattr(get_builder(document), 'inventory')(*args, **kwds)
  File "/home/king/Sage/git/sage/src/doc/common/builder.py", line 503, in _wrapper
    x.get(99999)
  File "/home/king/Sage/git/sage/local/lib/python/multiprocessing/pool.py", line 558, in get
    raise self._value
OSError: [matrices ] docstring of sage.matrix.matrix_gf2e_dense.unpickle_matrix_gf2e_dense_v0:15: WARNING: Block quote ends without a blank line; unexpected unindent.

I try to fix it.

@simon-king-jena
Copy link
Member

Reviewer: Simon King

@simon-king-jena
Copy link
Member

@simon-king-jena
Copy link
Member

@simon-king-jena
Copy link
Member

comment:10

Done. Turn """ into r""".

@simon-king-jena
Copy link
Member

comment:11

Why is trac automatically changing the branch back to the old value?? Odd.

@simon-king-jena
Copy link
Member

Changed commit from 26f359e to 32b2cb1

@simon-king-jena
Copy link
Member

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 19, 2015

Changed commit from 32b2cb1 to 1e47929

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 19, 2015

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

1e47929Change old into new style raise statement

@simon-king-jena
Copy link
Member

comment:13

You didn't actually introduce the old-style raise statements that the plugin is complaining about, but anyway I'm fixing them...

@simon-king-jena
Copy link
Member

comment:14

Since tests pass and the code and docstrings now seem fine, I'm giving it a positive review.

Unless of course you don't like my reviewer commits, or unless you worry about the patchbot reporting a slowdown in startup time (which I find hard to believe, given that you merely rename a module).

@vbraun
Copy link
Member

vbraun commented Sep 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants