Repository navigation
Store column views in CoefTable and reject offset arrays - #1026
Conversation
… arrays - Store views of the matrix columns instead of copies, as suggested in review. StatsBase never mutates `ct.cols`, and the `cols::Vector` constructor does not copy either. - Throw an `ArgumentError` for offset-indexed inputs in both constructors. Previously, offset columns were accepted and `show` and `iterate` failed later with a `BoundsError`, and offset matrices failed with a `MethodError`. - Tests: use `repr`, check exact error messages, cover offset arrays for both constructors, and restore indentation of a comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| function CoefTable(mat::AbstractMatrix,colnms::Vector,rownms::Vector, | ||
| pvalcol::Int=0,teststatcol::Int=0) | ||
| cols = Any[collect(col) for col in eachcol(mat)] | ||
| Base.require_one_based_indexing(mat) |
There was a problem hiding this comment.
Is there anything in this method that relies on one based indexing? Same for the other one.
There was a problem hiding this comment.
Methods for CoefTable such as the show method assume 1-based indexing, eg in
StatsBase.jl/src/statmodels.jl
Lines 166 to 170 in e5603c2
There was a problem hiding this comment.
But it can safely assume that because the fields are Arrays.
There was a problem hiding this comment.
The fields are Arrays, but their elements are indexed as well here, and those are not guaranteed to use 1-based indexing. The assumption also shows up in other places, e.g. in iterate:
StatsBase.jl/src/statmodels.jl
Line 105 in e5603c2
Follow-up to #1020, addressing review comments that were posted after it was merged.
Column storage.
CoefTable(::AbstractMatrix, ...)now storescollect(Any, eachcol(mat)), which holds views, instead of copying each column. Before #1020, theMatrixmethod copied viamat[:, i], but nothing depended on that. StatsBase only readsct.cols(inlength,eltype,iterateand bothshowmethods), and thecols::Vectorconstructor stores the caller's vectors without copying. #1020 had added tests that checked the copy; this PR removes them.Offset arrays. Both constructors now call
Base.require_one_based_indexingand throw anArgumentErrorfor offset-indexed input. Before this:CoefTable(Any[offset_vector, ...], ...)built the table, and thenshowanditeratefailed with aBoundsError, because they index1:nr.MethodError, becausecollect(Any, eachcol(mat))returns anOffsetVector{Any}, which is not aVector.Tests
repr("text/plain", x)/repr("text/markdown", x), as suggested in review.ArgumentErrormessages.Tested:
test/statmodels.jlpasses on Julia 1.10 and 1.13, and the full test suite passes on 1.13.🤖 Generated with Claude Code