Skip to content

Conversation

@lucaeg
Copy link
Contributor

@lucaeg lucaeg commented Jan 15, 2026

Vectorize the sampling rather than drawing one value at a time. This cosmetic change makes the code cleaner and easier to understand. In our view, drawing all samples for a parameter simultaneously is more natural and better aligned with standard sampling APIs (e.g., scipy).

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

@lucaeg lucaeg requested a review from oyvindeide January 15, 2026 12:50
@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.64%. Comparing base (0ac0ce1) to head (875eefb).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12648      +/-   ##
==========================================
- Coverage   90.66%   90.64%   -0.02%     
==========================================
  Files         429      429              
  Lines       29802    29806       +4     
==========================================
- Hits        27019    27018       -1     
- Misses       2783     2788       +5     
Flag Coverage Δ
cli-tests 37.56% <100.00%> (-0.01%) ⬇️
gui-tests 69.33% <100.00%> (-0.05%) ⬇️
performance-and-unit-tests 73.91% <100.00%> (-0.02%) ⬇️
test 38.07% <7.69%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 15, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing vectorize-sample-prior (875eefb) with main (f0d9de4)

Summary

✅ 22 untouched benchmarks

@lucaeg lucaeg added this to SCOUT Jan 16, 2026
@lucaeg lucaeg moved this to Ready for Review in SCOUT Jan 16, 2026
Copy link

@achaikou achaikou left a comment

Choose a reason for hiding this comment

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

I am sorry to crash the party uninvited, but I was out of stuff to do 😊

I am also missing some context in the commit description about the reason for the change (is there some problem to be solved or would code just look nicer?), so I have to guess 😄

Copy link

@achaikou achaikou left a comment

Choose a reason for hiding this comment

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

Am I right to assume that all those commits would be squashed?
I am just more accustomed to the fixup commit style 🙂

Btw, I am happy with the changes, but I don't feel like an ert-adult yet to click the "Approve" button for this PR 😄

Comment on lines +146 to +147
- num_realizations (int): Total number of realizations to generate the
reproducible sampling.

Choose a reason for hiding this comment

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

Got somewhat confused by the wording (thinking it was "number of realizations that would be generated").

Taking words from your other comment, maybe something like

Total number of realizations. Assures stable sampling for a given global_seed regardless of currently active realizations.

?

its sequence, effectively selecting the 'realization'-th sample from the
distribution.
- active_realizations (list[int]): indices of the realizations
to select from the stratified sampling vector; each must satisfy

Choose a reason for hiding this comment

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

Would vector now always be stratified? From the issue I understood stratifying strategy comes in addition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for Review

Development

Successfully merging this pull request may close these issues.

4 participants