Turing.jl icon indicating copy to clipboard operation
Turing.jl copied to clipboard

Fix error TuringOptimExt.jl with information matrix/vcov

Open smith-garrett opened this issue 1 year ago • 2 comments

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 avatar Feb 06 '24 14:02 smith-garrett

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

yebai avatar Feb 15 '24 10:02 yebai

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 avatar Feb 16 '24 07:02 smith-garrett

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?

smith-garrett avatar Feb 26 '24 11:02 smith-garrett

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

yebai avatar Mar 04 '24 16:03 yebai

@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 avatar Mar 05 '24 12:03 smith-garrett

@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. 🤞

smith-garrett avatar Mar 05 '24 14:03 smith-garrett

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 :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 05 '24 16:03 codecov[bot]

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%
<!-- Total: 0 2
Totals Coverage Status
Change from base Build 8140894641: 0.0%
Covered Lines: 0
Relevant Lines: 1367

💛 - Coveralls

coveralls avatar Mar 05 '24 16:03 coveralls

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!

smith-garrett avatar Mar 06 '24 11:03 smith-garrett

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.

yebai avatar Mar 06 '24 14:03 yebai