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

Fixes to AD backend usage in externalsampler #2223

Merged
merged 9 commits into from
May 18, 2024

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented May 16, 2024

I was playing around with an implementation of AutoMALA (see https://arxiv.org/abs/2310.16782 and Pigeons.jl) using AbstractMCMC.jl for a presentation I'm doing on Turing.jl-related stuff, and I ran into some issues with externalsampler.

Specifically, somewhere along the way, a LogDensityProblemsAD.ADgradient(...) of a DynamicPPL.LogDensityModel becomes just a DynamicPPL.LogDensityModel, i.e. we lose the ability to compute gradients.

We didn't catch this in our tests because we both AbstractHMC.jl and AbstractMH.jl default to using ForwardDiff if needed, and so they would use the correct adtype in the very first iteration and then subsequently switch to using their own defaults :upside_down_smiley:

@torfjelde
Copy link
Member Author

@yebai @devmotion @sunxd3 this should be a quick merge

Copy link
Member

@sunxd3 sunxd3 left a comment

Choose a reason for hiding this comment

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

looks good to me

@torfjelde
Copy link
Member Author

torfjelde commented May 17, 2024

Had to disable a few tests for MH, which doesn't actually follow the LogDensityProblems.jl interfacr properly (calls the method using NamedTuple instead of Vector). Two things here: a) all the NamedTuple cases are already easily supported by the internal MH sampler, so we're not losing any features here, and b) this is something that shoud be addressed in AdvancedMH.jl, not here.

EDIT: Tests should be passing now though, so if people are happy with the above, then we're gucci.

@torfjelde
Copy link
Member Author

torfjelde commented May 17, 2024

Uhmm the test failures seem very strange o.O I cannot reproduce this on my own machine; it's not windows, but I can't see how we can get undef behaviors depending on the OS o.O

EDIT: I lied 🤦 Was using the incorrect branch locally..

@torfjelde
Copy link
Member Author

It seems to be the same issue as JuliaFolds2/BangBang.jl#21 but we're using the correct version of BangBang.jl, indicating that the fix I did in JuliaFolds2/BangBang.jl#22 did not cover ReverseDiff.jl for some reason (though I'm also confused as to why we haven't caught this before).

@torfjelde
Copy link
Member Author

Okay, so now this PR should be passing. Disabled the test as mentioned in TuringLang/DynamicPPL.jl#612.

Once those tests have passed, this should be good to go.

Copy link

codecov bot commented May 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 0.00%. Comparing base (abb7981) to head (cbb1e59).
Report is 3 commits behind head on master.

Files Patch % Lines
src/mcmc/abstractmcmc.jl 0.00% 3 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2223   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          22      22           
  Lines        1528    1529    +1     
======================================
- Misses       1528    1529    +1     

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9140038894

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 0.0%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/mcmc/abstractmcmc.jl 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
ext/TuringOptimExt.jl 3 0.0%
Totals Coverage Status
Change from base Build 8989896440: 0.0%
Covered Lines: 0
Relevant Lines: 1529

💛 - Coveralls

@torfjelde torfjelde merged commit 53aff19 into master May 18, 2024
13 checks passed
@torfjelde torfjelde deleted the torfjelde/externalsampler-fixes branch May 18, 2024 15:48
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.

4 participants