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

Improve documentation of span method #12688

Closed
rbeezer mannequin opened this issue Mar 18, 2012 · 21 comments
Closed

Improve documentation of span method #12688

rbeezer mannequin opened this issue Mar 18, 2012 · 21 comments

Comments

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Mar 18, 2012

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. attachment: trac_12688_span_documentation.patch
  2. attachment: trac_12688_span_documentation-update.patch

Depends on #12541

CC: @novoselt

Component: linear algebra

Keywords: rd2, sd40.5

Author: Rob Beezer

Reviewer: Andrey Novoseltsev

Merged: sage-5.1.beta5

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

@rbeezer rbeezer mannequin added this to the sage-5.1 milestone Mar 18, 2012
@rbeezer rbeezer mannequin assigned jasongrout and williamstein Mar 18, 2012
@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 18, 2012

comment:2

See some discussion on #12541.

@rbeezer rbeezer mannequin changed the title Span method allows setting incompatible base ring, despite 'check=True' Improve documentation of span method Mar 18, 2012
@rbeezer rbeezer mannequin added p: minor / 4 and removed p: major / 3 labels Mar 18, 2012
@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 19, 2012

Attachment: trac_12688_span_documentation.patch.gz

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 19, 2012

Dependencies: 12541

@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 19, 2012

Changed keywords from none to rd2

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 19, 2012

Author: Rob Beezer

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 19, 2012

comment:3

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

@rbeezer rbeezer mannequin added the s: needs review label Mar 19, 2012
@novoselt
Copy link
Member

comment:4

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?

@novoselt
Copy link
Member

comment:5

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!

@novoselt
Copy link
Member

Reviewer: Andrey Novoseltsev

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 22, 2012

comment:6

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

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 22, 2012

comment:7

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

@novoselt
Copy link
Member

comment:8

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 ;-)

@novoselt
Copy link
Member

Changed keywords from rd2 to rd2, sd40.5

@novoselt
Copy link
Member

comment:9

#12688 comment:5 is still valid ;-)

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 25, 2012

Attachment: trac_12688_span_documentation-update.patch.gz

Address reviewer comments

@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 25, 2012

comment:10

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!

@jdemeyer
Copy link

jdemeyer commented Jun 2, 2012

Changed dependencies from 12541 to #12541

@jdemeyer
Copy link

Merged: sage-5.1.beta5

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

4 participants