oxinabox commented 2 years ago

This PR is a follow up to https://github.com/JuliaStats/PDMats.jl/pull/162#issuecomment-1135971969

It relaxes the StridedMatrix restrictions and uses eachindex, axes and broadcasting rather than 1:size(x,2) etc. A lot of places were using StridedMatrix with the assumption that it indexed safe with that scheme. (Which is not infact true, AFAIK, as any array extending DenseArray is a StridedArray. And DenseArray only requires contiguously in memory. But realisticallyOffsetArraysdoesn't actually subtypeDenseArray`, so doesn't really matter)

This PR changes:

1:size(x,2) to axes(x, 2) and 1:length(x) to eachindex(x) (this is done even when the types were not relaxed as it reads better)
rewrites some things entirely to just use broadcasting
@checkargs to now checks eachindex and axes, rather than length and size. (this makes certain they are indexed the same when we are using one eachindex rather than other. For eachindex we could use the 2 arg form but mostly we want to match a eachindex(a) against a axes(x,2), so can't)
Relaxes StridedArray into AbstractArray

It currently adds no tests, I am not familar with how this package is tested so I don't know if there is an easy way to test it e.g. with OffsetArrays or noncontigious SubArrays or something.

codecov-commenter commented 2 years ago

Codecov Report

Merging #163 (590b88e) into master (92a5178) will increase coverage by 0.34%. The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   90.33%   90.68%   +0.34%     
==========================================
  Files           8        8              
  Lines         414      397      -17     
==========================================
- Hits          374      360      -14     
+ Misses         40       37       -3

Impacted Files	Coverage Δ
src/pdsparsemat.jl	`94.64% <94.44%> (+0.30%)`	:arrow_up:
src/utils.jl	`95.65% <95.45%> (+3.98%)`	:arrow_up:
src/pdiagmat.jl	`98.76% <100.00%> (-0.22%)`	:arrow_down:
src/scalmat.jl	`96.66% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 92a5178...590b88e. Read the comment docs.

oxinabox commented 2 years ago

@devmotion what do you think. is this good to go now?

Or do you want

Tests of this directly
Actually supporting mixing of arrays with different axes

oxinabox commented 2 years ago

bump

matbesancon commented 2 years ago

@oxinabox can you also rebase on master?

oxinabox commented 2 years ago

The approach here of just demanding indexes are the same seems to be what is recommended by this thread. Rather than assuming that iteration order is order we should add together. https://discourse.julialang.org/t/discussion-on-why-i-no-longer-recommend-julia-by-yuri-vishnevsky/81151/314?u=oxinabox

matbesancon commented 2 years ago

@devmotion I'll let you have a last look but LGTM

JuliaStats / PDMats.jl

Remove StridedMatrix restriction and use modern index handling #163

Codecov Report