[RF] Fix problems with dataset generation from a nested RooSimultaneous with proto data
#12022
Conversation
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
When generating such a pdf from prototype data, the prototype needs to contain all the subcategories of the super-category, and it does so by checking the super-category servers. However, recently `RooSuperCategory` was changed to contain a `RooMultiCategory` internally, and the only reported direct server is the internal multi-category. This leads to a wrong generation (the prototype data is ignored, the gen context refers to the current labels). The solution that this commit suggests is to use `RooSuperCategory::inputCatList()` to inspect the internal categories instead of using the client-server interface. Actually, the client-server interface is not meant to be used for inspection like that, just for general dependency management in the evaluation. Closes root-project#12020.
* avoid code duplication * less manual memory management * more range-based loops
Plus some code modernization of the gen context implementations
guitargeek
force-pushed
the
issue-12020
branch
from
a9b8f57 to
87838c7
Compare
|
Starting build on |
guitargeek
force-pushed
the
issue-12020
branch
from
87838c7 to
9e17f70
Compare
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
In the `RooSimultaneous::genContext` function, the logic that figures out which `RooAbsGenContext` implementation to instantiate needs to know the list of all category components. The way to figure this out depends on the type of the index category, and this logic is also reused in the `RooAbsGenContext` implementations for the RooSimultaneous. That's why it is now factored out into a protected function called `RooSimultaneous::flattenCatList()`.
Add unit test that checks if the dataset generation from a nested RooSimultaneous with protodata containing the category values works. Covers GitHub issue root-project#12020.
In the `RooSimGenContex` and `RooSimSplitGenContex` classes, there is a place where the servers of a derived category are redirected. The `mustReplaceAll` flag was set to true here. This made sense when the internal categories were still the only servers of the `RooSuperCategory`, but since the refactor in root-project#5502 this is not the case anymore. Therefore, a harmless error is printed now, which should be avoided by setting the `mustReplaceAll` flag to `false`.
The `RooAbsArg::redirectServers()` funciton has an optional flag to print an error if not all servers listed in the new server set were replaced. However, if one was already using this `mustReplaceAll` flag, it means that the error is intentional and should be considered fatal. An exception should be thrown with it.
guitargeek
force-pushed
the
issue-12020
branch
from
9e17f70 to
6bb3d31
Compare
|
Starting build on |
lmoneta
approved these changes
Jan 13, 2023
Member
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thank you Jonas for fixing this problem!
52 tasks
This was referenced Mar 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
When generating such a pdf from prototype data, the prototype needs to
contain all the subcategories of the super-category, and it does so by
checking the super-category servers. However, recently
RooSuperCategorywas changed to contain aRooMultiCategoryinternally, and the only reported direct server is the internal
multi-category. This leads to a wrong generation (the prototype data is
ignored, the gen context refers to the current labels).
The solution that this commit suggests is to use
RooSuperCategory::inputCatList()to inspect the internal categoriesinstead of using the client-server interface. Actually, the
client-server interface is not meant to be used for inspection like
that, just for general dependency management in the evaluation.
Closes #12020.