Skip to content

pm.sample does not warn when using an unused argument is passed #3226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nbud opened this issue Oct 12, 2018 · 6 comments
Closed

pm.sample does not warn when using an unused argument is passed #3226

nbud opened this issue Oct 12, 2018 · 6 comments

Comments

@nbud
Copy link
Contributor

nbud commented Oct 12, 2018

Description of your problem

with model:
    trace = pm.sample(500, core=4, target_accept=0.95)

This code raises no exception or warning despite core and target_accept are unused arguments. This code won't have the intended effect and is hard to debug. The desired behaviour would be to raise a warning or an exception when unused arguments are passed.

For the record, the corrected code is:

with model:
    trace = pm.sample(500, cores=4, nuts_kwargs=dict(target_accept=0.95))

Versions and main components

  • PyMC3 Version: 3.5
  • Theano Version:
  • Python Version: 3.6
  • Operating system: Windows
  • How did you install PyMC3: conda
@fonnesbeck
Copy link
Member

We should remove nuts_kwargs and simply have step methods look for their arguments in kwargs

@ColCarroll
Copy link
Member

Would you prefer that to throwing errors when unused arguments are passed? I think it would be easy to support one or the other, but not both.

@springcoil
Copy link
Contributor

springcoil commented Oct 30, 2018

I think @fonnesbeck is right here. It'd be better API wise - by being clearer as a user - to remove nuts_kwargs than have errors.

@nbud
Copy link
Contributor Author

nbud commented Oct 30, 2018

Either way, it would be good to warn the user when an unused argument is passed, including for example passing target_accept or nuts_kwargs (depending on the chosen implementation) to a Step which does not use these arguments.

@eigenfoo
Copy link
Member

eigenfoo commented Dec 26, 2018

@fonnesbeck what about step_kwargs? Currently it looks like there are three ways to pass an option: through nuts_kwargs, through step_kwargs, or as a regular kwarg. I think the most sensible thing to is to agree on one, and deprecate the other two?

This issue is related to #3197

@fonnesbeck
Copy link
Member

Unifying them under kwargs would be my preference. I will try nd put a PR together later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants