Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#19251 closed enhancement (fixed)

LinearCode.basis() should be an immutable Sequence

Reported by: Nathann Cohen Owned by:
Priority: major Milestone: sage-6.9
Component: coding theory Keywords: beginner
Cc: David Lucas Merged in:
Authors: Johan Rosenkilde, Nathann Cohen Reviewers: David Lucas, Daniel Augot
Report Upstream: N/A Work issues:
Branch: b3dc9bf (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Johan Rosenkilde)

Right now the .basis() method returns just a list. This should instead be an immutable Sequence, retaining the type information in its .universe() method.

Change History (24)

comment:1 Changed 7 years ago by Nathann Cohen

Branch: public/19251
Status: newneeds_review

comment:2 Changed 7 years ago by git

Commit: f49c2040646aae994149a5a88a6ae4eb1d7a2941

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

f49c204trac #19251: A real matrix as LinearCode.basis()

comment:3 Changed 7 years ago by David Lucas

Status: needs_reviewneeds_info

Hello,

Just a question: if you're not adding a deprecation warning to this (which I agree with), what do you think about adding extra info to the docstring, like "Returns a basis of self as a matrix of the ambient space of self"?

comment:4 Changed 7 years ago by git

Commit: f49c2040646aae994149a5a88a6ae4eb1d7a29416cc755d0834799c340f881873acb9f6a57460a07

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

6cc755dtrac #19251: Better description

comment:5 Changed 7 years ago by Nathann Cohen

Status: needs_infoneeds_review

Here it is!

Nathann

comment:6 Changed 7 years ago by David Lucas

Reviewers: David Lucas
Status: needs_reviewpositive_review

Great!

As all tests pass, I'm giving the green bulb.

David

comment:7 Changed 7 years ago by Nathann Cohen

Thanks!

comment:8 Changed 7 years ago by Volker Braun

Status: positive_reviewneeds_work

Doctests fail.

Also, a matrix is not a basis. Matrices have row and column spans, its not clear which one you mean if you just return a matrix. This is how it is supposed to work:

sage: (QQ^3).span([(1,2,3)]).basis()
[
(1, 2, 3)
]
sage: _.universe()
Vector space of degree 3 and dimension 1 over Rational Field
Basis matrix:
[1 2 3]

comment:9 Changed 7 years ago by Nathann Cohen

Milestone: sage-6.9sage-duplicate/invalid/wontfix
Status: needs_workpositive_review

Never discuss with a release manager.

Nathann

comment:10 Changed 7 years ago by Johan Rosenkilde

There's still something of a bug, though, since the type returned by LinearCode.basis() is list, while what Volker writes about is a Sequence_generic, which exactly supports the universe method.

comment:11 Changed 7 years ago by Volker Braun

Status: positive_reviewneeds_info

+1 to returning an immutable Sequence instead of a list... are you going to implement that?

comment:12 Changed 7 years ago by Johan Rosenkilde

Authors: Nathann CohenJohan S. R. Nielsen, Nathann Cohen
Cc: Johan Rosenkilde removed
Description: modified (diff)
Milestone: sage-duplicate/invalid/wontfixsage-6.9
Status: needs_infoneeds_work
Summary: A real matrix as LinearCode.basis()LinearCode.basis() should be an immutable Sequence

Ok, I'm on it...

comment:13 Changed 7 years ago by git

Commit: 6cc755d0834799c340f881873acb9f6a57460a0791114c53c06cb430e3738a187a7fadafa7742879

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

7e0416dRevert basis() returning a matrix to returning a list
91114c5LinearCode.basis() returns a Sequence

comment:14 Changed 7 years ago by Johan Rosenkilde

Status: needs_workneeds_review

comment:15 Changed 7 years ago by Kevin Dilks

Status: needs_reviewneeds_work

One failing test in documentation due to formatting.

File "src/doc/en/prep/Quickstarts/Graphs-and-Discrete.rst", line 325, in doc.en.prep.Quickstarts.Graphs-and-Discrete
Failed example:
    D.basis()
Expected:
    [(1, 0, 1, 0, 1, 0, 1), (0, 1, 1, 0, 0, 1, 1), (0, 0, 0, 1, 1, 1, 1)]
Got:
    [
    (1, 0, 1, 0, 1, 0, 1),
    (0, 1, 1, 0, 0, 1, 1),
    (0, 0, 0, 1, 1, 1, 1)
    ]

comment:16 Changed 7 years ago by git

Commit: 91114c53c06cb430e3738a187a7fadafa774287969fb5c42f93e423a1a4a5be19a54cfdb2b20972d

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

9ccbb3eMerge remote-tracking branch 'trac/develop' into t/19251/public/19251
69fb5c4Fixed doctest in documentation text

comment:17 Changed 7 years ago by Johan Rosenkilde

Status: needs_workneeds_review

Missed that one - thanks. Rebased and doctest fixed.

comment:18 Changed 6 years ago by Johan Rosenkilde

Keywords: beginner added

comment:19 Changed 6 years ago by git

Commit: 69fb5c42f93e423a1a4a5be19a54cfdb2b20972db3dc9bfa5bb473c52d54dd507d39c512216c16bc

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

b3dc9bffixed conflict

comment:20 Changed 6 years ago by Daniel Augot

Hi

this is perfectly immutable to me. Nevertheless, I add to resolve a conflict when pulling.

Daniel

Version 0, edited 6 years ago by Daniel Augot (next)

comment:21 Changed 6 years ago by Daniel Augot

Status: needs_reviewpositive_review

comment:22 Changed 6 years ago by Johan Rosenkilde

Reviewers: David LucasDavid Lucas, Daniel Augot

comment:23 Changed 6 years ago by Volker Braun

Branch: public/19251b3dc9bfa5bb473c52d54dd507d39c512216c16bc
Resolution: fixed
Status: positive_reviewclosed

comment:24 Changed 5 years ago by Jeroen Demeyer

Authors: Johan S. R. Nielsen, Nathann CohenJohan Rosenkilde, Nathann Cohen
Commit: b3dc9bfa5bb473c52d54dd507d39c512216c16bc
Note: See TracTickets for help on using tickets.