Skip to content

solving PF memory growth issue #324

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

Merged
merged 3 commits into from
Oct 14, 2014
Merged

solving PF memory growth issue #324

merged 3 commits into from
Oct 14, 2014

Conversation

lileicc
Copy link
Contributor

@lileicc lileicc commented Oct 11, 2014

disable the use of CBN in DefaultPossibleWorld, which is used in PF and LWSampler. But LWSampler doesnot need CBN information.

@jxwuyi @datang1992

LWSampler. But LWSampler doesnot need CBN information.
@lileicc lileicc added this to the BLOG 0.10 release milestone Oct 11, 2014
@@ -895,12 +901,15 @@ private void updateParentsAndProbs(VarInfoUpdater updater) {
BayesNetVar var = (BayesNetVar) iter.next();
if ((var instanceof BasicVar) && (basicVarToValue.get(var) == null)) {
// var has been uninstantiated
cbn.removeNode(var);
varToLogProb.remove(var);
Copy link
Contributor

Choose a reason for hiding this comment

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

@lileicc Why do you want to remove this line? (About varToLogProb)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since I already put it in setValue(...)
when value is null, I already put varToLogProb.remove(var).

@datang1992
Copy link
Contributor

@lileicc LGTM with minor changes. But it will have several conflicts with the file changes in the branch #305 on the CBN part the partial world part.

@lileicc
Copy link
Contributor Author

lileicc commented Oct 12, 2014

@datang1992
Note that the changes will only affect those using DefaultPossibleWorld, won't affect the PartialWorldDiff.
Therefore, only LWSampler and ParticleFilter will be affected.

@datang1992
Copy link
Contributor

@lileicc
What I mean is that I changed something in the partial world file in the #305. Therefore there should be some conflicts.

@lileicc
Copy link
Contributor Author

lileicc commented Oct 12, 2014

@datang1992 @jxwuyi
ready to review again.
Previously the PF might still accumulate memory, due the the variables in dirtyVars in AbstractPartialWorld will be accumulated if there is not evidence.

@lileicc
Copy link
Contributor Author

lileicc commented Oct 14, 2014

@datang1992
As long as the underlying CBN is dynamical Bayesian network, it should be fine.
For open universe models, I donot think we support PF anyway.

@datang1992
Copy link
Contributor

OK. If we don't allow PF for OU models, then it LGTM.

@@ -104,7 +104,7 @@
*/
public WorldInProgress(Model model, Evidence evidence, int intBound,
int depthBound, int timestepBound) {
super(Collections.EMPTY_SET);
super(Collections.EMPTY_SET, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@datang1992 @lileicc Why record object is necessary in Gibbs and MH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exact answer is complicated. It is one way to record the usage graph.

@jxwuyi
Copy link
Contributor

jxwuyi commented Oct 14, 2014

Overall LGTM

lileicc added a commit that referenced this pull request Oct 14, 2014
solving PF memory growth issue, disable CBN recording objects in LWSampler and ParticleFilter
@lileicc lileicc merged commit d91695f into master Oct 14, 2014
@lileicc lileicc deleted the disable-cbn-in-default-pw branch October 14, 2014 05:19
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