Skip to content

fix the bug of MHSampler. #302

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

fix the bug of MHSampler. #302

wants to merge 11 commits into from

Conversation

datang1992
Copy link
Contributor

@lileicc @cberzan @chrisgioia64

As discussed in #301 , the current version of MHSampler is incorrect. The reason is that this implementation doesn't remove the unnecessary variables correctly when they try to build a new instantiation from an old one (some times it will delete less variables, and sometimes it will delete too much variables so that the evidences and queries are not supported).

In this modified version, this problem was fixed. Though this version has not been tested formally.

new PartialWorldDiff(world));
var.ensureDetAndSupported(context);
if (context.getCorrespondingVar() != null) {
evidenceAndQueries.addAll(context.getCorrespondingVar());
Copy link
Contributor

Choose a reason for hiding this comment

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

getDependentVars()

@lileicc
Copy link
Contributor

lileicc commented Aug 22, 2014

@chrisgioia64
to run experiments on all examples

@lileicc
Copy link
Contributor

lileicc commented Aug 22, 2014

Looks good.

}
}
}
for (Iterator iter = tmpWorld.getInstantiatedVars().iterator(); iter
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add type parameter to Iterator? Or you can use

for (Var var : ...getInstantiatedVars()) {
....
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lileicc
I don't think we need to add the type since you could see that we didn't add type parameter to Iterator everywhere else in the whole project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Chris will soon add type parameter to everywhere.
So you may start here.
If you use eclipse, you will see warnings. It is generally not a good idea
to keep the warnings.

On Fri, Aug 22, 2014 at 10:21 AM, Da Tang [email protected] wrote:

In src/main/java/blog/sample/GenericProposer.java:

  • PartialWorldDiff tmpWorld = new PartialWorldDiff(world);
  • for (Iterator iter = evidenceAndQueries.iterator(); iter.hasNext();) {
  •  BasicVar var = (BasicVar) iter.next();
    
  •  if (!tmpWorld.isInstantiated(var)) {
    
  •    return false;
    
  •  }
    
  •  TraceParentRecEvalContext context = new TraceParentRecEvalContext(
    
  •      tmpWorld);
    
  •  if (var instanceof VarWithDistrib) {
    
  •    ((VarWithDistrib) var).getDistrib(context);
    
  •    if (context.getNumCalculateNewVars() > 0) {
    
  •      return false;
    
  •    }
    
  •  }
    
  • }
  • for (Iterator iter = tmpWorld.getInstantiatedVars().iterator(); iter

@lileicc https://github.com/lileicc
I don't think we need to add the type since you could see that we didn't
add type parameter to Iterator everywhere else in the whole project.


Reply to this email directly or view it on GitHub
https://github.com/BayesianLogic/blog/pull/302/files#r16609606.

@datang1992
Copy link
Contributor Author

@lileicc The bug in Issue #303 seems has been fixed in my latest commit.

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