Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#19251 closed enhancement (fixed)

LinearCode.basis() should be an immutable Sequence

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.9
Component: coding theory Keywords: beginner
Cc: dlucas 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 jsrn)

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 6 years ago by ncohen

  • Branch set to public/19251
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by git

  • Commit set to f49c2040646aae994149a5a88a6ae4eb1d7a2941

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

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

comment:3 Changed 6 years ago by dlucas

  • Status changed from needs_review to needs_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 6 years ago by git

  • Commit changed from f49c2040646aae994149a5a88a6ae4eb1d7a2941 to 6cc755d0834799c340f881873acb9f6a57460a07

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

​6cc755dtrac #19251: Better description

comment:5 Changed 6 years ago by ncohen

  • Status changed from needs_info to needs_review

Here it is!

Nathann

comment:6 Changed 6 years ago by dlucas

  • Reviewers set to David Lucas
  • Status changed from needs_review to positive_review

Great!

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

David

comment:7 Changed 6 years ago by ncohen

Thanks!

comment:8 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_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 6 years ago by ncohen

  • Milestone changed from sage-6.9 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to positive_review

Never discuss with a release manager.

Nathann

comment:10 Changed 6 years ago by jsrn

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 6 years ago by vbraun

  • Status changed from positive_review to needs_info

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

comment:12 Changed 6 years ago by jsrn

  • Authors changed from Nathann Cohen to Johan S. R. Nielsen, Nathann Cohen
  • Cc jsrn removed
  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-6.9
  • Status changed from needs_info to needs_work
  • Summary changed from A real matrix as LinearCode.basis() to LinearCode.basis() should be an immutable Sequence

Ok, I'm on it...

comment:13 Changed 6 years ago by git

  • Commit changed from 6cc755d0834799c340f881873acb9f6a57460a07 to 91114c53c06cb430e3738a187a7fadafa7742879

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

  • Status changed from needs_work to needs_review

comment:15 Changed 6 years ago by kdilks

  • Status changed from needs_review to needs_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 6 years ago by git

  • Commit changed from 91114c53c06cb430e3738a187a7fadafa7742879 to 69fb5c42f93e423a1a4a5be19a54cfdb2b20972d

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

  • Status changed from needs_work to needs_review

Missed that one - thanks. Rebased and doctest fixed.

comment:18 Changed 5 years ago by jsrn

  • Keywords beginner added

comment:19 Changed 5 years ago by git

  • Commit changed from 69fb5c42f93e423a1a4a5be19a54cfdb2b20972d to b3dc9bfa5bb473c52d54dd507d39c512216c16bc

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

​b3dc9bffixed conflict

comment:20 Changed 5 years ago by danielaugot

Hi

this is perfectly immutable in my session. Nevertheless, I add to resolve a conflict when pulling.

I set it to positive review

Daniel

Last edited 5 years ago by danielaugot (previous) (diff)

comment:21 Changed 5 years ago by danielaugot

  • Status changed from needs_review to positive_review

comment:22 Changed 5 years ago by jsrn

  • Reviewers changed from David Lucas to David Lucas, Daniel Augot

comment:23 Changed 5 years ago by vbraun

  • Branch changed from public/19251 to b3dc9bfa5bb473c52d54dd507d39c512216c16bc
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:24 Changed 4 years ago by jdemeyer

  • Authors changed from Johan S. R. Nielsen, Nathann Cohen to Johan Rosenkilde, Nathann Cohen
  • Commit b3dc9bfa5bb473c52d54dd507d39c512216c16bc deleted
Note: See TracTickets for help on using tickets.