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

fixed bug in runCrossValidate for merging of MCMC configurations #1068

Merged
merged 20 commits into from
Sep 20, 2020

Conversation

danielturek
Copy link
Member

Fixes a bug in runCrossValidate, to correctly merge the sampler configuration lists from two different MCMC configurations.

Used conf$addSampler as was proposed by @perrydv, to do this in a robust manner.

Also added a test (using the dyes example), where cross validation now gives results that seem reasonable. The test checks for the exact numerical cross validation results.

@danielturek
Copy link
Member Author

@paciorek I suspect this will initially fail testing, due to the travis bug. But once that's fixed, this PR should pass (as it works for the cross-validation tests), and I'm hoping you can merge it into the upcoming release.

@perrydv
Copy link
Contributor

perrydv commented Sep 18, 2020

@paciorek Does this test failure look like just the travis issue we've been seeing?

@paciorek
Copy link
Contributor

Yeah, that is it. It didn't finish the other tests after numericTypes, but presumably those won't be an issue.

I should be fixing the precision error this morning.

@paciorek paciorek merged commit 493f791 into devel Sep 20, 2020
@paciorek paciorek deleted the fix_crossValidate branch September 20, 2020 23:54
@paciorek
Copy link
Contributor

@danielturek I'm trying to look into an issue raised on the users list about runCrossValidate and from what I can see in looking at that, it looks like the bug fix for runCrossValidate in this PR introduced a bug where the MCMC for all but the predictive loss is being done ONLY over the predictive nodes and not the parameters. Note that additional predictive samplers are assigned to the original conf configuration, but then the MCMC is built from modelMCMCConf, which contains only the predictive samplers.

@danielturek
Copy link
Member Author

@paciorek It appears this bug (embarrassing mistake) is mixing the changes made to conf versus modelMCMCConf. Is that your assessment also?

I'll open a PR fixing this as I believe it should be, but will wait for confirmation.

@paciorek
Copy link
Contributor

Yes, that's what I see.

@danielturek
Copy link
Member Author

This is being addressed in PR #1298.

Pending testing results.

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