Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add optional range for channels in ChannelCompatibilityCheck #760

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

IzaakWN
Copy link
Contributor

@IzaakWN IzaakWN commented May 23, 2022

With these changes, the user can optionally set the range of channel POIs as

combine -M ChannelCompatibilityCheck workspace.root -g chan1 -g chan2=-10,20 ...

This is useful in the case the POI hit the boundary in some channels, e.g. due to low sensitivity, large excess, ...

Alternatively, I also tried adding the channel POIs to autoMaxPOIs_ or autoBoundsPOIs_ by adding this snippet to ChannelCompatibilityCheck::runSpecific. This helped with boundary issues, but it's a bit ugly, and you get an error for e.g.

combine -M ChannelCompatibilityCheck workspace.root -g chan1 -g chan2 --autoMaxPOIs r,_ChannelCompatibilityCheck_r_chan2 ...
...
[#0] ERROR:InputArguments --  RooWorkspace::argSet(w) no RooAbsArg named "_ChannelCompatibilityCheck_r_chan2" in workspace

because this parameter does not exist before ChannelCompatibilityCheck::runSpecific.

@amarini amarini mentioned this pull request Oct 18, 2022
@nucleosynthesis
Copy link
Contributor

can we point this PR to main instead of 102x? I don't see why this can't be merged once thats done.

@IzaakWN IzaakWN changed the base branch from 102x to main October 10, 2023 09:45
@IzaakWN
Copy link
Contributor Author

IzaakWN commented Oct 10, 2023

Hi @nucleosynthesis, yes of course, done. However, I haven't validated it in this new branch.

@IzaakWN
Copy link
Contributor Author

IzaakWN commented Oct 10, 2023

Apologies for the noise, did you want to do a separate PR to main in addition to 102X? I just changed the target branch of this PR to main.

@nucleosynthesis
Copy link
Contributor

No, I meant just change the target, as you have done. Thanks!

@nucleosynthesis
Copy link
Contributor

Just tested and this works in main. The user could already change the ranges for all parameters using the --rMin, --rMax options but this indeed adds extra flexibility to change only some parameters in the split channel model.

@nucleosynthesis nucleosynthesis merged commit 965fc6a into cms-analysis:main Oct 10, 2023
1 check passed
@IzaakWN
Copy link
Contributor Author

IzaakWN commented Oct 10, 2023

Just tested and this works in main. The user could already change the ranges for all parameters using the --rMin, --rMax options but this indeed adds extra flexibility to change only some parameters in the split channel model.

Yeah, agreed. This was in context of EXO-19-016 where there the signal sensitivity varied greatly between different signal regions, depending on the model parameters, which caused some fit instability / problems with convergence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants