Use _init_parmas for MCMCThreads and MCMCDistributed too

[torfjelde]

Fixes #125

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d7c549f) 97.37% compared to head (4dbcb3f) 97.37%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #126   +/-   ##
=======================================
  Coverage   97.37%   97.37%           
=======================================
  Files           8        8           
  Lines         305      305           
=======================================
  Hits          297      297           
  Misses          8        8           

Files	Coverage Δ
src/sample.jl	96.64% <90.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[torfjelde]

So _first_or_nothing doesn't actually care if there are too many initial parameters 😕

IMO it seems like we want to raise an error in this case since it's otherwise very easy to accidentally do the incorrect thing.

[torfjelde]

So I just removed _first_or_nothing and added a simple check for the length of init_params.

Though this is then a breaking change

[devmotion]

Can we remove the collect call? It's not generally required and will lead to surprising allocations, e.g., the following cases would work just fine without collect (IIRC the docs even mention FillArrays if you want to specify the same parameters multiple times):

julia> collect(1:10)
10-element Vector{Int64}:
  1
  2
  3
  4
  5
  6
  7
  8
  9
 10

julia> collect(Zeros(10))
10-element Vector{Float64}:
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0
 0.0

Suggested change

	# We will use `getindex` later so we need to `collect`.
	_init_params = init_params !== nothing ? collect(init_params) : nothing

[torfjelde]

So I added it explicitly to cause allocations, but maybe it's not quite the right thing to do. We use getindex further down, and so if init_params is an iterator, e.g. Iterator.repeated, then this will fail unless we do something like collect first.

[torfjelde]

How about we just simplify the process by adding something like:

multiple_init_params(::Nothing, n) = Iterator.repeated(nothing, n)
function multiple_init_params(x, n)
    if length(x) != n
        throw(
            ArgumentError(
                "incorrect number of initial parameters (expected $n, received $(length(x))"
            ),
        )
    end

    return x
end

and then we just do

_init_params = multiple_init_params(init_params, n)

then we can just remove all the subsequent checks for nothing + use zip and avoid the getindex call.

[devmotion]

I think we should not generally change nothing to Iterator.repeated. Mainly, because it means we would not dispatch on nothing anymore and hence not use the map/pmap calls with reduced number of arguments anymore. IMO we really want to use these calls with one argument less since generally map, broadcast etc. is more optimized for AbstractArray than generic iterators.

For this reason I'm also not a fan of promoting and using Iterators.repeated. Maybe we should just demand that multiple sets of parameters are always provided as an AbstractVector? If you want something cheap, you can just use e.g. Fill(...) instead of Iterators.repeated - which in contrast to the latter supports the array interface.

[torfjelde]

IMO we really want to use these calls with one argument less since generally map, broadcast etc. is more optimized for AbstractArray than generic iterators.

Good point!

If you want something cheap, you can just use e.g. Fill(...) instead of Iterators.repeated - which in contrast to the latter supports the array interface.

Am happy with this 👍

[devmotion]

Could you update the docs (https://turinglang.org/AbstractMCMC.jl/dev/api/#Common-keyword-arguments) accordingly and remove Iterators.repeated there?

[torfjelde]

Will do 👍

[torfjelde]

@devmotion given that we're making a breaking change here, should we also throw in the renaming of init_params to initial_params as discussed in #119 ?

[devmotion]

Looks good. Adding using FillArrays: FillArrays to the tests should fix the test errors.

[torfjelde]

Aight, I'll merge this now, but let's delay the release until #119 has gone through to limit the number of breaking releases 👍

[devmotion]

But the PR here was marked as non-breaking anyway? 🤔 (I saw you already corrected the version bump 👍)

[torfjelde]

Wait, isn't minor version breaking? And this will be breaking, since we no longer accept previously accepted input types.

…

On Mon, Oct 2, 2023, 3:29 PM David Widmann ***@***.***> wrote: But the PR here was marked as non-breaking anyway? 🤔 (I saw you already corrected the version bump 👍) — Reply to this email directly, view it on GitHub <#126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUPZZH42IWLLTNVAE74M6LX5LFWFAVCNFSM6AAAAAA4XHLHYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBTGEZDQMJQGI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[devmotion]

No, minor versions are only breaking < 1.0.0. The next breaking release will be 5.0.0.

[torfjelde]

Ooooooh, crap. I clearly work too much with < 1.0.0 packages 😬

But aight, I'll bump to 5.0.0 then?

[devmotion]

👍