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 )
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:
Attachments (2)
Change History (15)
comment:1 Changed 7 years ago by
- Cc novoselt added
comment:2 Changed 7 years ago by
- 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
Changed 7 years ago by
comment:3 Changed 7 years ago by
- 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: ↓ 7 Changed 7 years ago by
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: ↓ 6 ↓ 10 Changed 7 years ago by
- 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
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: ↓ 8 Changed 7 years ago by
Replying to novoselt:
Perhaps these
span
andsubmodule
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
comment:9 Changed 7 years ago by
- Keywords sd40.5 added
http://trac.sagemath.org/sage_trac/ticket/12688#comment:5 is still valid ;-)
comment:10 in reply to: ↑ 5 Changed 7 years ago by
- 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
- Status changed from needs_review to positive_review
comment:12 Changed 7 years ago by
- Dependencies changed from 12541 to #12541
comment:13 Changed 7 years ago by
- Merged in set to sage-5.1.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
See some discussion on #12541.