Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models #541

torfjelde · 2023-10-06T19:51:44Z

This PR does two things:

rand(model) and others now, correctly, makes use of the context already attached to model. If we don't, stuff like condition will be completely ignored.
Removes overloads of rand(rng, NamedTuple, model) in TestUtils. This has annoyed me for quite some while because it means that we can't use properly test rand(model) since all of our test-models have this explicitly implemented (this was originally example_values in the PR where TestUtils but we decided to change it to overloading rand somewhere along the way).

Technically we can put change (1) in a PR on its own, but without change (2) it's non-trivial to test properly, hence why I'm doing both together.

The question is: is this then a breaking PR? Technically the existing rand implementations did exactly what they were supposed to do, i.e. when you called rand, you got back a NamedTuple of the variables as they appear in model, hence removing these implementations of rand (and thus hitting the default impl of rand for Model) should not change the behavior, as seen from the callers. In effect, we're just adding another method rand_prior_true 🤷

So maybe it's okay to just make this a patch-release? The "new" (i.e. now default) implementations of rand are also now tested to be the same as rand_prior_true.

EDIT: Nvm, this needs to be breaking since some of the models will have different results when calling rand now 😕

rand_prior_true so we can properly test rand

…r_true

src/test_utils.jl

coveralls · 2023-10-08T00:10:01Z

Pull Request Test Coverage Report for Build 6629476487

13 of 17 (76.47%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.09%) to 82.852%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/test_utils.jl	13	17	76.47%

Totals
Change from base Build 6619122585:	0.09%
Covered Lines:	2638
Relevant Lines:	3184

💛 - Coveralls

codecov · 2023-10-08T00:10:06Z

Codecov Report

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

Comparison is base (12e7c27) 82.76% compared to head (5ec07a9) 82.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   82.76%   82.85%   +0.08%     
==========================================
  Files          25       25              
  Lines        3180     3184       +4     
==========================================
+ Hits         2632     2638       +6     
+ Misses        548      546       -2

Files	Coverage Δ
src/model.jl	`88.34% <ø> (ø)`
src/test_utils.jl	`87.16% <76.47%> (+0.62%)`	⬆️

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

torfjelde · 2023-10-08T10:12:49Z

This should be good for a review, though it requires minor version bump, unfortunately 😕

torfjelde · 2023-10-09T09:38:24Z

So this is now passing and everything looks decent:)

Codecov is complaining but I don't understand how this can be true given that we've literally just:

Renamed a function
Added more tests.

torfjelde · 2023-10-09T09:38:57Z

But maybe we should wait with this until #542 and maybe #543 has gone through, as these are not breaking changes?

devmotion

LGTM 👍

Project.toml

test/varinfo.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…de/rand-fix

torfjelde · 2023-10-25T18:37:03Z

Tests should hopefully pass now that we yanked the AbstractMCMC release.

…ep existing compat) (#553) * Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models (#541) * preserve context from model in `rand` * replace rand overloads in TestUtils with definitions of rand_prior_true so we can properly test rand * removed NamedTuple from signature of TestUtils.rand_prior_true * updated references to previous overloads of rand to now use rand_prior_true * test rand for DEMO_MODELS * formatting * fixed tests for rand for DEMO_MODELS * fixed linkning tests * added missing impl of rand_prior_true for demo_static_transformation * formatting * fixed rand_prior_true for demo_static_transformation * bump minor version as this will be breaking * bump patch version * fixed old usage of rand * Update test/varinfo.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed another usage of rand --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Remove `tonamedtuple` (#547) * Remove dependencies to `tonamedtuple` * Remove `tonamedtuple`s * Minor version bump --------- Co-authored-by: Hong Ge <[email protected]> * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) --------- Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: CompatHelper Julia <[email protected]>

…t) (#551) * CompatHelper: bump compat for AbstractMCMC to 5, (keep existing compat) * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) (#552) Co-authored-by: CompatHelper Julia <[email protected]> * Update to AbstractMCMC 5 * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update sampler.jl * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) (#553) * Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models (#541) * preserve context from model in `rand` * replace rand overloads in TestUtils with definitions of rand_prior_true so we can properly test rand * removed NamedTuple from signature of TestUtils.rand_prior_true * updated references to previous overloads of rand to now use rand_prior_true * test rand for DEMO_MODELS * formatting * fixed tests for rand for DEMO_MODELS * fixed linkning tests * added missing impl of rand_prior_true for demo_static_transformation * formatting * fixed rand_prior_true for demo_static_transformation * bump minor version as this will be breaking * bump patch version * fixed old usage of rand * Update test/varinfo.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed another usage of rand --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Remove `tonamedtuple` (#547) * Remove dependencies to `tonamedtuple` * Remove `tonamedtuple`s * Minor version bump --------- Co-authored-by: Hong Ge <[email protected]> * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) --------- Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: CompatHelper Julia <[email protected]> * bump AbstractPPL version to 0.7 * Update AbstractPPL test dependency * add `Random.AbstractRNG` * Update sampler.jl (#557) * Update src/sampler.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Xianda Sun <[email protected]>

torfjelde added 5 commits October 6, 2023 18:09

preserve context from model in rand

2f65df1

replace rand overloads in TestUtils with definitions of

96bac66

rand_prior_true so we can properly test rand

removed NamedTuple from signature of TestUtils.rand_prior_true

979b747

updated references to previous overloads of rand to now use rand_prio…

54d48fd

…r_true

test rand for DEMO_MODELS

73ff641

github-actions bot reviewed Oct 6, 2023

View reviewed changes

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

torfjelde added 3 commits October 7, 2023 01:42

formatting

e92c4bf

fixed tests for rand for DEMO_MODELS

b88c2ea

fixed linkning tests

be7a4b1

torfjelde changed the title ~~Fix for rand + remove overloads of rand for testing models~~ Fix for rand + replace overloads of rand with rand_prior_true for testing models Oct 7, 2023

added missing impl of rand_prior_true for demo_static_transformation

e2fec3d

github-actions bot reviewed Oct 7, 2023

View reviewed changes

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

torfjelde added 2 commits October 7, 2023 19:50

formatting

9bbd05c

fixed rand_prior_true for demo_static_transformation

05e7a0a

bump minor version as this will be breaking

de75531

torfjelde mentioned this pull request Oct 23, 2023

Remove tonamedtuple #547

Merged

devmotion approved these changes Oct 24, 2023

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

torfjelde added 2 commits October 24, 2023 15:19

Merge branch 'master' into torfjelde/rand-fix

5de35af

bump patch version

e987cd9

devmotion approved these changes Oct 24, 2023

View reviewed changes

yebai assigned yebai and unassigned yebai Oct 24, 2023

fixed old usage of rand

ad5f2b8

github-actions bot reviewed Oct 24, 2023

View reviewed changes

test/varinfo.jl Outdated Show resolved Hide resolved

Update test/varinfo.jl

23486bc

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

torfjelde added 2 commits October 24, 2023 17:01

fixed another usage of rand

a7cbee8

Merge remote-tracking branch 'origin/torfjelde/rand-fix' into torfjel…

5ec07a9

…de/rand-fix

yebai enabled auto-merge October 25, 2023 21:26

yebai added this pull request to the merge queue Oct 25, 2023

Merged via the queue into master with commit 2e8adf4 Oct 26, 2023
11 of 13 checks passed

yebai deleted the torfjelde/rand-fix branch October 26, 2023 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models #541

Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models #541

torfjelde commented Oct 6, 2023 •

edited

Loading

coveralls commented Oct 8, 2023 •

edited

Loading

codecov bot commented Oct 8, 2023 •

edited

Loading

torfjelde commented Oct 8, 2023

torfjelde commented Oct 9, 2023 •

edited

Loading

torfjelde commented Oct 9, 2023

devmotion left a comment

torfjelde commented Oct 25, 2023

Fix for rand + replace overloads of rand with rand_prior_true for testing models #541

Fix for rand + replace overloads of rand with rand_prior_true for testing models #541

Conversation

torfjelde commented Oct 6, 2023 • edited Loading

coveralls commented Oct 8, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6629476487

💛 - Coveralls

codecov bot commented Oct 8, 2023 • edited Loading

Codecov Report

torfjelde commented Oct 8, 2023

torfjelde commented Oct 9, 2023 • edited Loading

torfjelde commented Oct 9, 2023

devmotion left a comment

Choose a reason for hiding this comment

torfjelde commented Oct 25, 2023

Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models #541

Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models #541

torfjelde commented Oct 6, 2023 •

edited

Loading

coveralls commented Oct 8, 2023 •

edited

Loading

codecov bot commented Oct 8, 2023 •

edited

Loading

torfjelde commented Oct 9, 2023 •

edited

Loading