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

Fix step method assignment #3203

Closed
wants to merge 8 commits into from

Conversation

eigenfoo
Copy link
Member

Closes #3197

This PR does away with the assigned_vars set, and instead uses the selected_steps dictionary all the way. It was a fairly clunky implementation to begin with, so this shouldn't be too much of a surprise.

What was strange was the line that I deleted: step = step_class(vars=vars, **args)
I think this line was supposed to instantiate step_class, but the problem is that step_class is already an instantiated step method. So as far as I can tell, this method never really worked at all! Not sure how we got here, but test_sampling.py is passing locally, so...

Asides from that, some readability edits: more explicit variable names, clearer/more docstrings, friendlier error messages.

@eigenfoo eigenfoo changed the title Fix assign step methods Fix step method assignment Sep 17, 2018
pymc3/sampling.py Outdated Show resolved Hide resolved
args = step_kwargs.get(step_class.name, {})
used_keys.add(step_class.name)
step = step_class(vars=vars, **args)
args = step_kwargs.get(step.name, {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is args actually getting used now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... it doesn't look like it is at all. Perhaps this is related to the Travis errors I'm getting. Will look into this later this week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that step_class is an object at this point, and not the class? I think there is something suspicious here. If it's not a class, should it be? If it is a class, how could we pass args?

@eigenfoo
Copy link
Member Author

Travis failures look real. A good deal of them are just missing name attributes from some samplers, which I added. I'm more worried about the failing tests from test_step.py. I'll take another look sometime later this week.

@twiecki twiecki added the WIP label Sep 17, 2018
pymc3/sampling.py Outdated Show resolved Hide resolved
@twiecki
Copy link
Member

twiecki commented Oct 27, 2018

@eigenfoo What's the status of this?

@twiecki twiecki mentioned this pull request Oct 27, 2018
@eigenfoo
Copy link
Member Author

@twiecki stale. I'd be happy to continue working on this, but don't really have the bandwidth right now. I don't want to block #3239, so closing for now (and hopefully will revisit in the future).

@eigenfoo eigenfoo closed this Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants