Opened 6 years ago

Last modified 4 years ago

#19251 closed enhancement

LinearCode.basis() should be an immutable Sequence — at Version 12

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.9
Component: coding theory Keywords: beginner
Cc: dlucas Merged in:
Authors: Johan S. R. Nielsen, Nathann Cohen Reviewers: David Lucas
Report Upstream: N/A Work issues:
Branch: public/19251 (Commits, GitHub, GitLab) Commit: 6cc755d0834799c340f881873acb9f6a57460a07
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 (12)

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...

Note: See TracTickets for help on using tickets.