Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Restrict values_as_in_model API #778

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

penelopeysm
Copy link
Member

Looking through the codebase, one realises that values_as_in_model only really serves one purpose, which is to extract the values as seen in the model for a given varinfo.

This PR therefore restricts the API of this function to do precisely that (and nothing more). Specifically, it:

  • removes the rng argument, which would previously cause the function to sample again. (If you really wanted to sample, I think you should sample first and then pass that VarInfo to values_as_in_model, i.e. values_as_in_model(model, true, VarInfo(rng, model))
  • forces the user to pass a VarInfo argument (previously it would default to generating a blank VarInfo, which would error if rng wasn't also provided).

This means that the only signatures allowed are

values_as_in_model(::Model, ::Bool, ::AbstractVarInfo)
values_as_in_model(::Model, ::Bool, ::AbstractVarInfo, ::AbstractContext)

Closes #767

@coveralls
Copy link

coveralls commented Jan 15, 2025

Pull Request Test Coverage Report for Build 12793059716

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.007%) to 86.338%

Files with Coverage Reduction New Missed Lines %
src/contexts.jl 2 30.69%
src/varinfo.jl 3 84.33%
Totals Coverage Status
Change from base Build 12708918770: -0.007%
Covered Lines: 3735
Relevant Lines: 4326

💛 - Coveralls

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.25%. Comparing base (e673b69) to head (6039253).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
- Coverage   86.26%   86.25%   -0.01%     
==========================================
  Files          36       36              
  Lines        4332     4330       -2     
==========================================
- Hits         3737     3735       -2     
  Misses        595      595              

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

@penelopeysm penelopeysm force-pushed the py/values-as-in-model branch from c2916fa to 6039253 Compare January 15, 2025 16:47
@penelopeysm penelopeysm requested a review from sunxd3 January 15, 2025 17:25
Copy link
Member

@sunxd3 sunxd3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@penelopeysm penelopeysm added this pull request to the merge queue Jan 16, 2025
Merged via the queue into master with commit 938a69d Jan 16, 2025
20 checks passed
@penelopeysm penelopeysm deleted the py/values-as-in-model branch January 16, 2025 15:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValuesAsInModelContext cleanup
3 participants