Skip to content
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

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

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

Comments

@nbud
Copy link
Contributor

@nbud 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

This comment has been minimized.

Copy link
Member

@fonnesbeck fonnesbeck commented Oct 12, 2018

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

@ColCarroll

This comment has been minimized.

Copy link
Member

@ColCarroll ColCarroll commented Oct 22, 2018

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

This comment has been minimized.

Copy link
Member

@springcoil 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

This comment has been minimized.

Copy link
Contributor Author

@nbud 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

This comment has been minimized.

Copy link
Member

@eigenfoo 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

This comment has been minimized.

Copy link
Member

@fonnesbeck fonnesbeck commented Dec 26, 2018

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
6 participants
You can’t perform that action at this time.