-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix cut acceptance plotting #135
Conversation
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 @ghusheng for catching this. but I feel like we can just keep using cut_func
as before to make the codes concise. I made a commit. Can you please check if it makes sense to you?
saltax/match/utils.py
Outdated
for cut_oi in all_cut_list: | ||
selected_events_cut_oi = selected_events[apply_single_cut(selected_events, cut_oi)] | ||
selected_events_cut_oi = selected_events[cut_func(selected_events, cut_oi)] | ||
# Efficiency curves with Clopper-Pearson uncertainty estimation | ||
interval = binomtest(len(selected_events_cut_oi), len(selected_events)).proportion_ci() | ||
result_dict[cut_oi][i] = len(selected_events_cut_oi) / len(selected_events) |
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.
Hi Lanqing! If we still keep the code like this, I am not sure the N-1 cut acceptance is correct. should be len(selected_events_all_cut) / len(selected_events_cut_oi)?
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.
Great thanks for the sanity check now I wake up. Is it better now?
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.
Haha, the thing is that if you change in this way then it won't work for the single cut acceptance. These two need to be treated differently :)
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.
You are right. I buy it. Let me revert.
for more information, see https://pre-commit.ci
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 again @ghusheng for capturing this important bug. Now it should be fixed.
np |
Fix the
n_minus_1 plotting
option in theget_cut_eff