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.

Sign up for GitHub

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

Jump to bottom

[WIP] Fix for timeout in graph_model #3460

Merged
merged 4 commits into from
May 5, 2019
Merged

[WIP] Fix for timeout in graph_model #3460

merged 4 commits into from
May 5, 2019

Conversation

rpgoldman
Copy link
Contributor

See #3458

@rpgoldman rpgoldman changed the title Fix for timeout in graph_model [WIP] Fix for timeout in graph_model May 3, 2019
@rpgoldman
Copy link
Contributor Author

This PR is almost right, I think, but somehow by changing the ancestor computation, I have changed the computation of the dimensions in graph rendering.

lucianopaz
lucianopaz requested changes May 4, 2019
View reviewed changes
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

Given that you observed a change in the computed dimensions, could you add a new test to the test suite that checks if the computed dimensions match with what should be returned?

pymc3/model_graph.py Outdated
# this contains all of the variables in the model EXCEPT var...
vars: List[var] = set(self.var_list)
vars.remove(var)

upstream = self._ancestors(var, func)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would completely remove self._ancestors and replace it with the stack_search that you did

@rpgoldman
Copy link
Contributor Author

@lucianopaz actually the dimension change was not an issue in the breadth first search -- it revealed an error in the code I used to debug the search! That's a relief. I will see if I can propose some tests, but I don't understand the test suite very well.
I will try to make the change you suggest today.

@rpgoldman
Copy link
Contributor Author

@lucianopaz I have just looked over the existing tests for the model grapher, and they check the structure, including the identification of plates. So I think they are sufficient in their present form. There is no obvious test to add based on the fix, because the old version didn't compute the wrong thing; it simply didn't complete. But I am open to suggestions.

Note: if this PR looks good, I should squash it before it's merged.

lucianopaz
lucianopaz requested changes May 5, 2019
View reviewed changes
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

Everything looks fine to me. No other tests should be needed. Once _ancestors is removed, just add a line to the release notes under maintenance and the PR should be ready to merge

pymc3/model_graph.py Outdated
@@ -40,24 +47,32 @@ def _ancestors(self, var, func, blockers=None):
return set([j for j in ancestors([func], blockers=blockers) if j in self.var_list and j != var])
Copy link
Contributor

Choose a reason for hiding this comment

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

_ancestors now seems to be unused, right? If so, it should be removed.

@rpgoldman
Revise _get_ancestors to use BFS.
f76df8c 
Previously, `get_ancestors()` used a powerset computation, which would fail on large models. Replaced with breadth-first search.
@rpgoldman
Simplify _get_ancestors().
8981c7e 
Per @lucianopaz. remove the initial special case/
@rpgoldman
Copy link
Contributor Author

@lucianopaz OK, I think this is done now.

@lucianopaz
Copy link
Contributor

Great! Thanks a lot @rpgoldman! Once the tests pass, I'll merge

lucianopaz
lucianopaz requested changes May 5, 2019
View reviewed changes
Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

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

Oh, I had forgotten that you still need to add a line to RELEASE-NOTES.md under the maintenance section referencing the fixed issue

@rpgoldman
Remove unused _ancestors method
9f3bba0 
New Breadth-first search version of _get_ancestors no longer uses it.
Also added some type annotations to clarify.
@rpgoldman
Release note for PR pymc-devs#3460
ca13c44
@rpgoldman
Copy link
Contributor Author

@lucianopaz Pushed the documentation and removed an unused import causing the check failure.

@lucianopaz lucianopaz merged commit 05e3c39 into pymc-devs:master May 5, 2019
@lucianopaz
Copy link
Contributor

Thanks again @rpgoldman!

@lucianopaz lucianopaz mentioned this pull request May 5, 2019
Closed
@rpgoldman
Copy link
Contributor Author

@lucianopaz No problem! I'm glad to be able to give even a little something back in exchange for all I've gotten from the community!

@twiecki
Copy link
Member

twiecki commented May 6, 2019

Thanks @rpgoldman!

@rpgoldman rpgoldman deleted the grapher-dfs branch August 14, 2019 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucianopaz lucianopaz lucianopaz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rpgoldman @lucianopaz @twiecki