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

Basic rewrite of the package 2023 edition Part II: Location-scale variational families #51

Merged
merged 9 commits into from
Dec 20, 2023

Conversation

Red-Portal
Copy link
Member

This is a partial pull request of #45.
(The original PR is now severely outdated.)

The content of Part II is as follows:

  • Add the location-scale variational family
  • Add constructors for Gaussian variational families.
  • Optimize AD paths for the location-scale family. (Refer to the original PR for benchmarks.)

@Red-Portal Red-Portal requested review from torfjelde and yebai and removed request for torfjelde December 9, 2023 08:11
Copy link

codecov bot commented Dec 9, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (576259a) 81.15% compared to head (bbfac2a) 85.92%.

Files Patch % Lines
src/families/location_scale.jl 96.72% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   81.15%   85.92%   +4.77%     
==========================================
  Files          10       11       +1     
  Lines         138      199      +61     
==========================================
+ Hits          112      171      +59     
- Misses         26       28       +2     

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

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Thanks @Red-Portal -- it looks good overall. I have a few minor comments below.

src/AdvancedVI.jl Show resolved Hide resolved
src/families/location_scale.jl Show resolved Hide resolved
Comment on lines 36 to 37
z = rand(q)
@test eltype(z) == realtype
Copy link
Member

Choose a reason for hiding this comment

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

A bit redundant with above logpdf tests.

Suggested change
z = rand(q)
@test eltype(z) == realtype

Copy link
Member Author

Choose a reason for hiding this comment

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

@yebai Though without it, it seems codecov complanes that _logpdf is not covered. Should I still remove the tests fore _logpdf?

test/interface/location_scale.jl Outdated Show resolved Hide resolved
@yebai yebai merged commit 9ebfc3f into TuringLang:master Dec 20, 2023
8 checks passed
@Red-Portal Red-Portal deleted the rewriting_advancedvi_locscale branch September 10, 2024 04:02
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