use axes() and eachindex() to better support arbitrary AbstractArray
#1550
Conversation
There was a problem hiding this comment.
Thanks for the PR!
In general, I think it would be good to
- not try to fix all array indexing in one PR, and
- not to mix functional changes (new features or breaking changes) with these bug fixes.
By not trying to go through all of Distributions in one go, fixes and reviews, and hence releases, will be much quicker. Even more so since for every fix we should add a test with e.g. OffsetArrays to make sure we don't accidentally break stuff again at some point in the future.
Keeping the PR focused to these bug fixes will also make reviewing easier and avoid accidentally breaking downstream code.
|
@palday Any plans to finish this PR? ;-) |
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
|
@nalimilan I started working on this again while other stuff was compiling and running today, but the tests -- even on |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1550 +/- ##
==========================================
- Coverage 85.91% 85.91% -0.01%
==========================================
Files 142 142
Lines 8579 8574 -5
==========================================
- Hits 7371 7366 -5
Misses 1208 1208
☔ View full report in Codecov by Sentry. |
|
Thanks. CI passes on x86 though so looks like we can move ahead. |
| @@ -334,7 +335,7 @@ function _mixlogpdf!(r::AbstractArray, d::AbstractMixtureModel, x) | |||
|
|
|||
| # in the mean time, add log(prior) to lp and | |||
| # update the maximum for each sample | |||
| for j = 1:n | |||
| for j in eachindex(r) | |||
There was a problem hiding this comment.
This seems problematic as j is used to index into lp_i which is a view of a Matrix column. We should use two different indices or for (j, lp_ij) in zip(eachindex(m), lp_i) as below. r isn't relevant here anyway.
| end | ||
| end | ||
| end | ||
|
|
||
| @inbounds for j = 1:n | ||
| @inbounds for j in eachindex(r) |
| for k in axes(P, 1) | ||
| pk = P[k, iP] | ||
| @inbounds μ[k] += pk * wi | ||
| @inbounds γ[k] += pk * pk * wi |
There was a problem hiding this comment.
Should use a different index for μ and γ as these may not have the same indices as P.
Starts the process of fixing #1265.
Other things that I saw while doing this:
AbstractMatrix{Float64}orAbstractArray{Float64}.I did start changing someAbstractArraytoAbstractVectorwhen it was clear that there was an implicit assumption of one dimension.Matrix{T}.