fix: correctly pass parameters to sample from alternative#190
fix: correctly pass parameters to sample from alternative#190jonas-eschle merged 4 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
=======================================
Coverage 92.38% 92.39%
=======================================
Files 26 26
Lines 1274 1275 +1
=======================================
+ Hits 1177 1178 +1
Misses 97 97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in toy generation where parameters were not being correctly passed to zfit samplers. The root cause was that zfit samplers ignore the current parameter state set via context managers and require parameters to be passed directly via the param_values argument. The fix also includes updates for NumPy compatibility and adds comprehensive regression tests for simultaneous fits.
- Fixes parameter passing to samplers by replacing context manager with direct
param_valuesargument - Adds parameter filtering for simultaneous fit scenarios where different samplers use different parameters
- Updates NumPy API usage from deprecated
tostring()totobytes()for hash computation - Switches test assertions from
assert_array_equaltoassert_allclosefor floating point comparisons
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/hepstats/utils/fit/sampling.py | Core fix: replaces set_values() context manager with direct param_values passing and adds parameter filtering for simultaneous fits |
| src/hepstats/hypotests/parameters.py | Updates deprecated NumPy method from tostring() to tobytes() in hash function |
| tests/hypotests/test_calculators.py | Adds regression test to verify toys are generated at specified POI values, not data's best fit |
| test_simultaneous_fit.py | Comprehensive test suite for simultaneous fits with various parameter sharing scenarios |
| tests/modeling/test_bayesianblocks.py | Changes assertions from assert_array_equal to assert_allclose for floating point tolerance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s.resample() # do not pass parameters as arguments as it will fail in simultaneous fits | ||
| for s in samplers: | ||
| # Filter params to only those relevant to this sampler (for simultaneous fits) | ||
| sampler_param_names = set(s.params.keys()) |
There was a problem hiding this comment.
The sampler_param_names set is computed inside the ntoys loop (line 60) but only depends on the sampler s, not the iteration variable i. This means the same computation is repeated unnecessarily for each toy. Consider computing this once per sampler before the loop to improve performance, especially when generating many toys.
2 participants
No description provided.