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

Add support for Turing v0.33 #189

Merged
merged 20 commits into from
Aug 16, 2024
Merged

Conversation

mhauru
Copy link
Contributor

@mhauru mhauru commented Jun 25, 2024

As discussed in TuringLang/Turing.jl#2268. This reimplements the relevant parts of Turing.optim_problem and Turing.optim_functions that were removed in Turing v0.33. The new Turing MAP/MLE interface is unfortunately a bit too streamlined for Pathfinder, which needs access to the internal OptimizationFunctions.

@sethaxen, feel free to take over and edit the code to your taste. Please also check the logic of how I'm transforming any initial values provided, and generating defaults from the prior when none are provided; I think the test suite wouldn't catch it if I had done it wrong.

Copy link
Member

@sethaxen sethaxen left a comment

Choose a reason for hiding this comment

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

Thanks @mhauru for the PR! I made a number of suggestions. You don't need to implement these, but they do reflect changes I'd make, so if you see a problem with them, please let me know.

There does seem to be an issue where the log-det-jacobian of the bijector is ignored, which I've commented on with an example.

ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
@sethaxen
Copy link
Member

sethaxen commented Jul 4, 2024

@mhauru with #178 and #198 I made it so we use ADTypes internally and LogDensityProblemsAD for LogDensityProblems inputs. Then support for Turing models only requires 1) converting the Turing model to a LogDensityProblem (using DynamicPPL.LogDensityFunction) and 2) converting unconstrained draws to constrained draws (using a utility function). This greatly simplified the implementations. I also removed our own machinery for getting parameter names in favor of Turing.Inference.getparams, which also allows us to support models with dynamic constraints.

Tests now check for correctness (including of the Jacobian) by ensuring that an IID normal transformed with a bijector to a constrained space can be exactly fitted with Pathfinder. This is possible because Pathfinder will always fit an IID normal exactly, and in such cases the log-density of the unconstrained distribution is equivalent to that of the IID normal. These tests fail for previous Pathfinder versions.

If you have a chance, take a look and let me know if anything looks off.

Copy link
Contributor Author

@mhauru mhauru left a comment

Choose a reason for hiding this comment

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

Looks great, much neater this way when the extension just needs to make the LogDensityProblem. I left a couple of small comments. Thanks for taking over this!

test/integration/Turing/runtests.jl Show resolved Hide resolved
ext/PathfinderTuringExt.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.85%. Comparing base (93253e7) to head (823a515).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   80.70%   86.85%   +6.14%     
==========================================
  Files          13       13              
  Lines         622      601      -21     
==========================================
+ Hits          502      522      +20     
+ Misses        120       79      -41     

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

@sethaxen sethaxen merged commit 0ecd772 into mlcolab:main Aug 16, 2024
16 checks passed
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.

2 participants