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

Fix error TuringOptimExt.jl with information matrix/vcov #2167

Merged
merged 10 commits into from Mar 6, 2024

Conversation

smith-garrett
Copy link
Contributor

I've been using the MLE/MAP estimation functionality (defined here) with some Turing models, and I've come across an error in the informationmatrix function, extended from StatsBase.

In the function definition, the Hessian of the log likelihood or joint is first calculated on the unconstrained scale. Then that matrix is inverted and returned as the observed Fisher information matrix for the MLE/MAP. StatsBase.vcov is then extended to basically just be an alias for informationmatrix.

This seems to be incorrect. According to Wikipedia, the observed information matrix is just the negative of the Hessian (not the inverse), and the variance-covariance matrix is the inverse of the information matrix. This pull request fixes these two spots in the code.

smith-garrett and others added 2 commits February 6, 2024 15:30
Fix errors in information matrix/variance-covariance matrix calculation for MLEs/MAPs
@yebai
Copy link
Member

yebai commented Feb 15, 2024

@cpfiffer, since you worked on this code, can you take a look, please?

Copy link
Member

@cpfiffer cpfiffer left a comment

Choose a reason for hiding this comment

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

Yep, this seems correct! Thanks for the fix 😁

@smith-garrett
Copy link
Contributor Author

My pleasure! I've been using Turing a lot in the last couple years, so I'm happy to be able to contribute a little bit. :)

It looks like there are a couple of tests still failing. I can have a look at those next week and update the pull request when they pass.

@smith-garrett
Copy link
Contributor Author

I've now updated the tests, but they still seem to be failing. Maybe I missed a step somewhere in getting the CI pipeline to run the new test versions?

@yebai
Copy link
Member

yebai commented Mar 4, 2024

@smith-garrett I can confirm CI tests are running on the most recent releases of DynamicPPL and related numerical libraries. Maybe take another look?

Usually, the information matrix is the negative of the Hessian of the log likelihood, but because Optim minimizes the negative of the log likelihood, we don't need to take the negative here. The Hessian is the info matrix in this context.
…n test

The analytical values for the information and vcov matrices are now calculated using the standard equations. Added separate tests for both matrices.
@smith-garrett
Copy link
Contributor Author

@yebai Thanks for the update. I think my tests were off, but I think I have fixed them now. The vcov and information matrices now have separate tests based comparing the results to analytical calculations.

@smith-garrett
Copy link
Contributor Author

@yebai Sorry, there were some silly errors in my last commit. I think I've fixed everything now, as the tests are all passing on my computer. 🤞

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

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

Project coverage is 0.00%. Comparing base (0b56415) to head (427095b).

Files Patch % Lines
ext/TuringOptimExt.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2167   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          21      21           
  Lines        1368    1367    -1     
======================================
+ Misses       1368    1367    -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 8157507510

Details

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

Changes Missing Coverage Covered Lines Changed/Added Lines %
ext/TuringOptimExt.jl 0 2 0.0%
Totals Coverage Status
Change from base Build 8140894641: 0.0%
Covered Lines: 0
Relevant Lines: 1367

💛 - Coveralls

@smith-garrett
Copy link
Contributor Author

There are still two failing tests, but they don't seem to have to do with the information matrix/vcov stuff as far as I can tell. I hope I haven't introduced a new error somewhere!

@yebai yebai merged commit 3a315ce into TuringLang:master Mar 6, 2024
13 checks passed
@yebai
Copy link
Member

yebai commented Mar 6, 2024

Thanks @smith-garrett -- there are some tests that have a stringent numerical error threshold which fails occasionally due to stochasticity! These have gone away after running the CIs.

@smith-garrett smith-garrett deleted the patch-1 branch March 7, 2024 07:49
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.

None yet

4 participants