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

JuliaBUGS support #303

Merged
merged 34 commits into from
Dec 20, 2024
Merged

JuliaBUGS support #303

merged 34 commits into from
Dec 20, 2024

Conversation

miguelbiron
Copy link
Collaborator

@serenlee I've created this new PR with a more structured approach for giving support for JuliaBUGS models. It is based on Julia extension mechanisms, which allows us to keep precompilation times manageable for most users.
Now you can still keep working on your own PRs. But once you are comfortable with the code, we should migrate it to this PR.

Also, beyond the extension boilerplate code, I added a function that can automatically produce a prior model to use as default reference. It works by manipulating the underlying graph of the target, removing nodes related to observations. I think it is 80% there but needs testing.

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.36%. Comparing base (07806d3) to head (1ba7742).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
ext/PigeonsJuliaBUGSExt/utils.jl 93.33% 2 Missing ⚠️
ext/PigeonsJuliaBUGSExt/interface.jl 96.00% 1 Missing ⚠️
ext/PigeonsJuliaBUGSExt/invariance_test.jl 90.90% 1 Missing ⚠️
src/Pigeons.jl 0.00% 1 Missing ⚠️
src/explorers/invariance_test.jl 0.00% 1 Missing ⚠️
src/log_potentials/log_potentials.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
+ Coverage   87.23%   87.36%   +0.13%     
==========================================
  Files         103      107       +4     
  Lines        2570     2644      +74     
==========================================
+ Hits         2242     2310      +68     
- Misses        328      334       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexandrebouchard
Copy link
Member

Awesome!! Glad to hear the prior can be automatically extracted after all...

@serenlee
Copy link
Collaborator

serenlee commented Dec 2, 2024

@miguelbiron Thanks!

I have also been interacting with the JuliaBUGS team. I will work on the JuliaBUGS target and push it to this PR branch.

@sunxd3
Copy link

sunxd3 commented Dec 4, 2024

Hi guys, thanks for working on this. Please just let me know if anything is needed, happy to make additions on the JuliaBUGS end!

@miguelbiron
Copy link
Collaborator Author

miguelbiron commented Dec 4, 2024

Thank you @sunxd3! Actually I'd like your opinion on our approach to evaluating the prior. Context: this is an important requirement for PT methods. From what we understand of your code, there is no parallel in JuliaBUGS to the PriorContext construct in DynamicPPL. Therefore, there is no way of using evaluate!! to compute just the prior part (i.e., with no likelihood terms).

My solution was to automatically extract a prior JuliaBUGS model from the joint model. This is achieved by pruning non-parameter nodes from the full graph. Then we use the smaller graph to build the prior model, with which we can easily get prior logpdfs.

Question: do you think this is a reasonable approach? And do you think JuliaBUGS will in the future support prior logpdf computation?

Edit: Actually it would be even better to have access to a TemperedContext that would let you evaluated the logpdf of the tempered posterior for any beta>=0. I.e., logpdf_prior(x) + beta*loglikelihood(x).

@sunxd3
Copy link

sunxd3 commented Dec 5, 2024

TuringLang/JuliaBUGS.jl#247

I am not taking the contextual evaluation design for JuliaBUGS now because I felt the complexity is not worth it yet. So I just adde a function in the above PR. (at the cost of duplicated code)

The interface will very likely to change, but I'll make sure to update here once the potential happen.

@miguelbiron
Copy link
Collaborator Author

Oh wow thank you so much! I'll test the new interface as soon as possible.

@miguelbiron
Copy link
Collaborator Author

Ok, switched to the new interface and things seem to be proceeding smoothly! Thanks again @sunxd3 .

On a related note @sunxd3, I was thinking you could avoid having to maintain duplicated code by redefining evaluate!! to just call the tempered version with temp = 1.

@miguelbiron
Copy link
Collaborator Author

Need to make initialization more stable (currently depends on global RNG); might explain this sole fail

https://github.com/Julia-Tempering/Pigeons.jl/actions/runs/12261789152/job/34209633693?pr=303#step:8:1128

@serenlee
Copy link
Collaborator

@alexandrebouchard
Is the current version good to merge?

There is a to-do list

@miguelbiron
Copy link
Collaborator Author

@serenlee the MPI issue is fixed now. There was an issue with the way I was creating the private models for the replicas, which was affecting the MPI stuff downstream.

@miguelbiron
Copy link
Collaborator Author

The parallelism invariance check (checked_round input param) is the one part not working, but just because the equality check of BUGSModels is non-trivial.

@miguelbiron
Copy link
Collaborator Author

@serenlee it'd be nice to have simple reproducible example for the failure you detected.

@miguelbiron
Copy link
Collaborator Author

@serenlee there is a problem with your new definition of the incomplete data model

https://github.com/Julia-Tempering/Pigeons.jl/actions/runs/12399506324/job/34614570875?pr=303#step:8:1038

@miguelbiron
Copy link
Collaborator Author

Pushed a fix that should close #296

@miguelbiron miguelbiron linked an issue Dec 20, 2024 that may be closed by this pull request
@miguelbiron
Copy link
Collaborator Author

Ok everything in @serenlee 's list has been fixed (except for the Gibbs sampler which is not our problem). I'll merge this now.

@miguelbiron miguelbiron merged commit 72a523b into main Dec 20, 2024
10 checks passed
@miguelbiron miguelbiron deleted the juliabugs branch December 20, 2024 17:32
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.

Error/warning behaviour for NaN logprobs
4 participants