-
Notifications
You must be signed in to change notification settings - Fork 24
Editing plotting script to include stat uncertainties #492
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
base: master
Are you sure you want to change the base?
Conversation
bryates
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating this, @abasnet97! I have a few comments/questions, but nothing major.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #492 +/- ##
==========================================
- Coverage 24.31% 24.01% -0.31%
==========================================
Files 36 36
Lines 5605 5676 +71
==========================================
Hits 1363 1363
- Misses 4242 4313 +71
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:
|
anpicci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abasnet97 Thank you very much!
Overall, this is quite good. I have one main comment about the strategy for displaying systematic and statistical errors. As mentioned in a comment in the code, I think we should allow users to choose which uncertainties they want to display in the plots. I would avoid hardcoding assumptions in this regard. Additionally, I would prefer to use the booleans unblind, plot_syst_err, and plot_stat_err independently to avoid hardcoding assumptions about what we want to display in different scenarios.
I understand that adding this functionality to the code might require some time for development. If you and @bryates prefer to merge the code as is and implement my suggestion in a separate PR, I'm fine with that.
…n stat uncertainty calculation
|
@anpicci @bryates I agree with Andrea's comments about having the functionality to display stat error and syst. error separately via some command line option. And that is exactly what I was talking about when I said that some aspects of the code will have to be revisited (not because they are wrong but because people's taste and preference will evolve). So my opinion on this is that if someone can run this code to do some tests and it does not throw any obvious errors or display incorrect error bands, we can probably proceed with merging this. Then, a different PR can be made or changes can be introduced based on what the group prefers eventually. What do you guys think? |
Sounds good. Let's merge this PR and open a new one. @abasnet97 do you have time to work on it since you're technically not in our group anymore? Otherwise, @anpicci did you want to implement the suggestions? |
bryates
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @abasnet97. I still had a few minor questions, but overall this looks great so I'll mark it as approved.
Hi @bryates, @anpicci, sorry I missed this. But I think the leftover work is now up to Andrea (or anyone else using this script) to modify things as per their taste. There are some loose ends, but I think they will be super easy to fix once the group decides on what contributions to include in the stat uncertainties. Let me know what you guys think. |
No worries. I agree, @anpicci can handle this or pass it along to someone else in the group. Thanks again for getting this PR started! |
|
@bryates I am fine with having this PR merged and follow up on the open items |
…mulator Ensure AnalysisProcessor hist list matches accumulator
|
Hi @bryates , I resolved some conflicts, but everything should be ok |
|
Thanks @anpicci and @abasnet97 for handling this! @Andrew42 and @sscruz do you have any comments? |
This PR edits the
make_cr_and_sr_plots.pyto correctly handle the statistical uncertainties in the plot. In the existing code, the stat err is calculated using the nominal histograms, and this PR introduces changes to use thesumw2histograms to calculate the stat err. The systematic errors are also properly accounted for (although it should be emphasized that they were already properly handled in the existing code).Caveat
sumw2values of all the background processes (meaning the sumw2 of EFT signal processes are not used in the calculation. One should revisit this in the future cause for the purpose of limit setting, we do not use stat error of the background MC samples at all. The only stat errors that we use are associated with data-driven pieces (nonprompt leptons). The reason this PR does not only include nonprompt lepton stat error is cause we currently do not do nonprompt lepton estimation in the4lchannel, and so it was deemed that many unnecessary changes would have to be introduced to handle this exception. So my recommendation is that this be REVISITED in the future.ttAanalysis specific changes. That is because it is not relevant to the main branch yet.