BUG: `max_treedepth` argument to `pm.sample` throws error

[dehorsley]

Describe the issue:

Supplying argument max_treedepth to pm.sample throws a ValueError.

Bisect suggests it was introduced in this commit: 0660efa

Reproduceable code example:

import pymc as pm

with pm.Model() as m:
    pm.Normal("mu")
    pm.sample(max_treedepth=15)

Error message:

ValueError: Unused step method arguments: {'max_treedepth'}

PyMC version information:

5.4.0

Context for the issue:

No response

[jahall]

Seems it is related to this change from 0660efa

The previous behaviour would have dropped the max_treedepth kwarg as it overwrote the entire dict. Now it is kept in there.

Not sure what the desired behaviour should be, but from the docs it seems like the only valid additional kwarg names are start, target_accept or the lowercase name of a sampler e.g. nuts, metropolis, etc. So this should work:

import pymc as pm

with pm.Model() as m:
    pm.Normal("mu")
    pm.sample(nuts={"max_treedepth": 15})

But to fix the issue I guess there are two options:

1. Raise a more informative error when passed invalid kwargs
2. Have bespoke handling of a direct max_treedepth kwarg where it is re-attached to the nuts kwarg dict (similar to what is done for target_accept)

[ricardoV94]

Have bespoke handling of a direct max_treedepth kwarg where it is re-attached to the nuts kwarg dict (similar to what is done for target_accept)

I remember we made an exception for target_accept, but in general we don't want kwargs to be passed like that

[ricardoV94]

Raise a more informative error when passed invalid kwargs

Yes, we could definitely do that, and even suggest how to fix it

[jahall]

^ something like this?

[dehorsley]

My thinking is that this broke backwards compatibility. I'm not sure what the project's goals are around preserving backwards compatibility, but my preference would keep the API (or at least restore to pre v5.1.1) and at least give a deprecation warning. Given there's a bit of code and are a few examples on the pymc.io site that use the direct kwarg to sample form, it's a small but frustrating thing for a beginner to hit. Happy to hear other opinions.

[jahall]

Looks to me like the behaviour before the commit above was that this argument was silently ignored.

[dehorsley]

@jahall, yeah I think you're right! Looks like 0660efa fixed a bug made in 18bbcbb. max_treedepth kwarg to sample worked up to v4.1.4.