Show warning message if initial_parameters is passed instead of initial_params

[shravanngoswamii]

Closes #175

[github-actions]

AbstractMCMC.jl documentation for PR #176 is available at:
https://TuringLang.github.io/AbstractMCMC.jl/previews/PR176/

[yebai]

thanks @shravanngoswamii!

[penelopeysm]

Thanks @shravanngoswamii -- there is one thing missing here, namely that single-chain sampling doesn't issue a warning.

using AbstractMCMC
struct M <: AbstractMCMC.AbstractModel end
struct S <: AbstractMCMC.AbstractSampler end
AbstractMCMC.step(rng, ::M, ::S, state=nothing; kwargs...) = rand(rng), nothing
sample(M(), S(), 5; initial_parameters=42.42)

Now if you were to copy-paste the warning inside the method for mcmcsample, my guess is that you will probably find that multiple-chain sampling

sample(M(), S(), MCMCThreads(), 10, 3; initial_parameters=42.42)

will issue a warning (nchains+1) times, because the keyword arguments are passed straight on to single-chain sample.

So, if I'm not wrong, in the multiple-chain sampling when you are performing this check you should also make sure that initial_parameters is removed from kwargs after issuing the warning, e.g. using:

kwargs = BangBang.delete!!(kwargs, :initial_parameters)

(I hunted around for a base Julia way of removing a key from a NamedTuple, but it's not released yet: JuliaLang/julia#55270 so I think BangBang is needed.)

[shravanngoswamii]

Hi @penelopeysm, the kwargs are of type Base.Pairs, and I think BangBang.delete!! doesn’t work with Base.Pairs (from kwargs...). I tried it, but it didn’t work, so I used manual filtering instead. Maybe I’m wrong, though—I haven’t worked with these things as much.

Also, I noticed something in the DynamicPPL PR that @yebai mentioned in #175. You used hasproperty(kwargs, :initial_parameters), but I think hasproperty doesn’t work with Base.Pairs (what kwargs... produces). You should use haskey instead. Additionally, test coverage seems to be missing for this warning (Codecov shows "2 Missing" lines), so maybe that wasn't detected.

[penelopeysm]

Oh, yes, good catch. Sure, the DynamicPPL bit has similar problems to what I described here too (including with removing the keyword argument). But if the warning is in AbstractMCMC, then there's no need for the DynamicPPL one, and I can just remove it.

[shravanngoswamii]

But if the warning is in AbstractMCMC, then there's no need for the DynamicPPL one, and I can just remove it.

Okay, let's merge it!

[penelopeysm]

Will merge when CI passes, thanks @shravanngoswamii!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (fc74dd4) to head (539d06b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #176   +/-   ##
===========================
===========================

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.