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

Implement missing posterior_optima and likelihood_optima for a test model. #503

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

yebai
Copy link
Member

@yebai yebai commented Jul 20, 2023

Fix some test failures in TuringLang/Turing.jl#2018. Also, transfer some utility functions for test models from Turing to this repo.

src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
src/test_utils.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@coveralls
Copy link

coveralls commented Jul 20, 2023

Pull Request Test Coverage Report for Build 5612537845

  • 0 of 32 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.9%) to 76.302%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/test_utils.jl 0 32 0.0%
Totals Coverage Status
Change from base Build 5611139343: -0.9%
Covered Lines: 2051
Relevant Lines: 2688

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.92 ⚠️

Comparison is base (74849d4) 77.22% compared to head (4c81032) 76.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   77.22%   76.30%   -0.92%     
==========================================
  Files          22       22              
  Lines        2656     2688      +32     
==========================================
  Hits         2051     2051              
- Misses        605      637      +32     
Impacted Files Coverage Δ
src/test_utils.jl 83.19% <0.00%> (-8.20%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sunxd3
Copy link
Member

sunxd3 commented Jul 21, 2023

TuringLang/Turing.jl#2049 is failing because the functions were moved here
(c.f. https://github.com/TuringLang/Turing.jl/blob/e32bb71accb4d71dfc5f0377984a00aa5d643c3a/test/modes/OptimInterface.jl#L160 and https://github.com/TuringLang/Turing.jl/blob/e32bb71accb4d71dfc5f0377984a00aa5d643c3a/test/modes/OptimInterface.jl#L191)

These are easy to fix, my concern is that we never refer to Optim interface in DynamicPPL. In my opinion, it is debatable if we should separate the Optim interface and the ground truth to test the interface.

If moving the functions here while leaving everything else put is the way to go , I am happy to see this and TuringLang/Turing.jl#2049 through

@yebai
Copy link
Member Author

yebai commented Jul 21, 2023

These are easy to fix, my concern is that we never refer to Optim interface in DynamicPPL. In my opinion, it is debatable if we should separate the Optim interface and the ground truth to test the interface.

I don't have a strong preference here, but the majority of test-related functionalities for such models are in DynamicPPL. So it might be better to keep all these codes in the same place. In fact, one bug was caused by missing the posterior_optima for newly added models. It shows potential confusion for developers.

@yebai yebai enabled auto-merge July 21, 2023 10:18
@yebai yebai added this pull request to the merge queue Jul 21, 2023
Merged via the queue into master with commit 0d5c463 Jul 21, 2023
10 of 13 checks passed
@yebai yebai deleted the hg/add-mle-map-estimators branch July 21, 2023 10:42
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.

3 participants