[RF] Always save generated parameters in RooMCStudy - #23141
Merged
guitargeek merged 1 commit intoAug 25, 2026
Conversation
Fix three related problems around RooMCStudy::genParDataSet(), reported in GitHub issue root-project#9490 and its discussion thread: 1. The generated parameter values were only recorded when internal constraints were used via Constrain(), despite the documentation promising that they are saved. The _genParData dataset is now always created and filled with one entry per generated toy. 2. Parameters constrained with ExternalConstraints() were not sampled from the constraint p.d.f.s for each toy, because only RooAbsPdf::getAllConstraints() was consulted, which cannot see constraint terms that are not part of the model. The external constraint p.d.f.s are now included in the per-toy sampling, with the constrained parameters found by intersecting the constraint observables with the model parameters. Passing the constraints inside FitOptions() remains available to apply them in the fit only. 3. When some toy fits failed, merging the generated parameter values into the fit parameter dataset failed with "ERROR: datasets have different size", because _genParData has one entry per generated toy while _fitParData only gets entries for converged fits. The merge now goes through a copy that only contains the toys whose fit converged, so the "<name>_gen" columns stay row-aligned with fitParDataSet(), while genParDataSet() keeps the entries of all generated toys (including the failed ones, so no information is dropped). Further consequences of the implementation: - genParDataSet() columns are no longer renamed in place to "<name>_gen"; only the merged copies are. This also fixes repeated runs accumulating "_gen" suffixes. - The merge only happens for runs that both generate and fit, and only when the parameters are sampled from constraint p.d.f.s. This avoids clobbering the "<name>_gen" columns published by RooRandomizeParamMCSModule and keeps the fitParDataSet() schema unchanged for unconstrained studies. Fit-only runs previously merged the columns in reverse toy order, silently producing wrong pulls; they now emit a warning instead. - _genParData is filled after the processBeforeGen() module hook, so it records the values actually used for generation. - generate() and generateAndFit() reset _genParData like the other result containers. Closes root-project#9490 🤖 Done with the help of AI
Test Results 23 files 23 suites 3d 17h 21m 55s ⏱️ For more details on these failures, see this check. Results for commit 05d4de3. |
dpiparo
self-requested a review
August 25, 2026 12:15
dpiparo
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix three related problems around RooMCStudy::genParDataSet(), reported in GitHub issue #9490 and its discussion thread:
The generated parameter values were only recorded when internal constraints were used via Constrain(), despite the documentation promising that they are saved. The _genParData dataset is now always created and filled with one entry per generated toy.
Parameters constrained with ExternalConstraints() were not sampled from the constraint p.d.f.s for each toy, because only RooAbsPdf::getAllConstraints() was consulted, which cannot see constraint terms that are not part of the model. The external constraint p.d.f.s are now included in the per-toy sampling, with the constrained parameters found by intersecting the constraint observables with the model parameters. Passing the constraints inside FitOptions() remains available to apply them in the fit only.
When some toy fits failed, merging the generated parameter values into the fit parameter dataset failed with "ERROR: datasets have different size", because _genParData has one entry per generated toy while _fitParData only gets entries for converged fits. The merge now goes through a copy that only contains the toys whose fit converged, so the "_gen" columns stay row-aligned with fitParDataSet(), while genParDataSet() keeps the entries of all generated toys (including the failed ones, so no information is dropped).
Further consequences of the implementation:
Closes #9490
🤖 Done with the help of AI