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

Tweak sample counts and other details in tests #2433

Merged
merged 25 commits into from
Dec 14, 2024

Conversation

mhauru
Copy link
Member

@mhauru mhauru commented Dec 6, 2024

This PR makes a number of small changes to the tests

  • Lowers the sample counts of many tests.
  • Adds a few comments, including some TODO notes saying we should add an explanatory comment.
  • Uses StableRNGs more. Still not using them everywhere, but I added them in many places where stability matters.
  • Removes unnecessary calls to Random.seed!. Every @testset resets the seed anyway.
  • Removes some dead or commented out tests that weren't doing anything.
  • Adds verbose = true to many of the top-level @testsets, to get some timing numbers even when tests pass.
  • Adds info prints in the beginning of some of the longer-running testsets, saying which test is starting.
  • Adds some more subtestsets to some of the longer-running ones, to get more granular info about what fails.
  • Fixes one commented-out test to actually do something.

This started with me thinking that since we do a lot of samples in many tests, and our test suite is quite slow, clearly we should reduce the sample counts if we can. What I learned in the process is that most of the tests that do sampling are negligble compared to the tests that use particle methods. Anything involving PG/CSMC tends to take 10-100 times longer than comparable tests with other samplers.

In the end I've tried to adjust iteration counts only when either the count truly doesn't matter (we are not checking stats of the chain), or when doing so actually has a noticable, though not necessarily huge, impact on run time. I may have slipped a few times, and reduced something that didn' really make a difference. Also, when reducing iteration counts I tried to check that they pass if I pick a few different seeds, to make sure I'm not making them very brittle to future changes.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.41%. Comparing base (c0a4ee9) to head (544f9be).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2433   +/-   ##
=======================================
  Coverage   86.41%   86.41%           
=======================================
  Files          22       22           
  Lines        1575     1575           
=======================================
  Hits         1361     1361           
  Misses        214      214           

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

@coveralls
Copy link

coveralls commented Dec 7, 2024

Pull Request Test Coverage Report for Build 12321043702

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.8%) to 76.801%

Totals Coverage Status
Change from base Build 12110228665: -1.8%
Covered Lines: 1205
Relevant Lines: 1569

💛 - Coveralls

@mhauru mhauru changed the title Reduce iterations in tests Tweak sample counts and other details in tests Dec 13, 2024
@mhauru mhauru marked this pull request as ready for review December 13, 2024 12:54
@mhauru mhauru requested a review from penelopeysm December 13, 2024 12:54
@mhauru
Copy link
Member Author

mhauru commented Dec 13, 2024

I'm hoping CI will now pass, except for Mooncake stack overflows. See updated OP for what this PR does.

test/mcmc/ess.jl Outdated Show resolved Hide resolved
test/mcmc/hmc.jl Outdated Show resolved Hide resolved
Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

Also, if you could bump Mooncake compat to 0.4.61, that would hopefully take care of CI 👍

@penelopeysm
Copy link
Member

I can approve and merge once CI passes :)

@mhauru mhauru requested a review from penelopeysm December 13, 2024 18:29
@penelopeysm penelopeysm self-assigned this Dec 13, 2024
@penelopeysm
Copy link
Member

penelopeysm commented Dec 14, 2024

Some Ubuntu tests are looking like they will time out, but I'm going to merge on the basis that the tests pass on Windows and macOS and the root cause for the timeout is known (TuringLang/DynamicPPL.jl#750) and will presumably be fixed soon (TuringLang/DynamicPPL.jl#751).

@penelopeysm penelopeysm merged commit 365fe16 into master Dec 14, 2024
64 of 69 checks passed
@penelopeysm penelopeysm deleted the mhauru/reduce-test-iterations branch December 14, 2024 00:54
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