Opened 7 years ago

Closed 7 years ago

#12688 closed enhancement (fixed)

Improve documentation of span method

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-5.1
Component: linear algebra Keywords: rd2, sd40.5
Cc: novoselt Merged in: sage-5.1.beta5
Authors: Rob Beezer Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12541 Stopgaps:

Description (last modified by rbeezer)

I discovered this while working on #12541. Documentation says the check will "coerce entries of gens into base field." This does not appear to be happening, and should. (Edit: this is an integral span of rational vectors, a construction that is intended, so improving the documentation might be in order.)

sage: (QQ^2).span(gens=[vector(QQ, [1,1/2])], base_ring=ZZ, check=True)
Free module of degree 2 and rank 1 over Integer Ring
Echelon basis matrix:
[  1 1/2]

Depends: #12541

Apply:

  1. trac_12688_span_documentation.patch
  2. trac_12688_span_documentation-update.patch

Attachments (2)

trac_12688_span_documentation.patch (4.7 KB) - added by rbeezer 7 years ago.
trac_12688_span_documentation-update.patch (1.2 KB) - added by rbeezer 7 years ago.
Address reviewer comments

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by novoselt

  • Cc novoselt added

comment:2 Changed 7 years ago by rbeezer

  • Description modified (diff)
  • Priority changed from major to minor
  • Summary changed from Span method allows setting incompatible base ring, despite 'check=True' to Improve documentation of span method
  • Type changed from defect to enhancement

See some discussion on #12541.

Changed 7 years ago by rbeezer

comment:3 Changed 7 years ago by rbeezer

  • Authors set to Rob Beezer
  • Dependencies set to 12541
  • Description modified (diff)
  • Keywords rd2 added
  • Status changed from new to needs_review

Patch is documentation only. More thorough, and more in keeping with current documentation standards.

comment:4 follow-up: Changed 7 years ago by novoselt

I'll look at it today/tomorrow if nobody beats me.

A side remark (for probably the next ticket?): while working on #12541 I had to fix several pieces of code that looked almost the same. In fact, I have removed exactly the same *wrong* lines from six places or so in different specializations of free modules. Perhaps these span and submodule methods can be somehow consolidated, e.g. have span only in the base class which calls _span of subclasses for actual work after all the typechecking and conversion. Or, if it is really necessary to have them everywhere, maybe there can be only 1 or 2 good long docstrings, as the one provided by Rob here, and others should just link to it?

comment:5 follow-ups: Changed 7 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to needs_work

Little picks:

  • "INPUTS" should be without "S" according to guidelines.
  • There also should be "OUTPUT" and perhaps it can precisely state here where the resulting submodule lives.

Otherwise looks good to me!

comment:6 in reply to: ↑ 5 Changed 7 years ago by rbeezer

Replying to novoselt:

Little picks:

  • "INPUTS" should be without "S" according to guidelines.
  • There also should be "OUTPUT" and perhaps it can precisely state here where the resulting submodule lives.

Thanks, Andrey. I forget these in my rush to wrap it up. I'll get an updated patch up soon, but it might be a couple days.

Rob

comment:7 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by rbeezer

Replying to novoselt:

Perhaps these span and submodule methods can be somehow consolidated

I have not looked, but it would not surprise me if there was some beneficial consolidation. Maybe at Bug Days in a few weeks I'll tackle this one.

Rob

comment:8 in reply to: ↑ 7 Changed 7 years ago by novoselt

Replying to rbeezer:

Replying to novoselt:

Perhaps these span and submodule methods can be somehow consolidated

I have not looked, but it would not surprise me if there was some beneficial consolidation. Maybe at Bug Days in a few weeks I'll tackle this one.

Rob

Great, I'll be there too ;-)

comment:9 Changed 7 years ago by novoselt

  • Keywords sd40.5 added

Changed 7 years ago by rbeezer

Address reviewer comments

comment:10 in reply to: ↑ 5 Changed 7 years ago by rbeezer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to novoselt:

Thanks again - comments addressed in "update" patch.

Rob

Little picks:

  • "INPUTS" should be without "S" according to guidelines.
  • There also should be "OUTPUT" and perhaps it can precisely state here where the resulting submodule lives.

Otherwise looks good to me!

comment:11 Changed 7 years ago by novoselt

  • Status changed from needs_review to positive_review

comment:12 Changed 7 years ago by jdemeyer

  • Dependencies changed from 12541 to #12541

comment:13 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.1.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.