-
Notifications
You must be signed in to change notification settings - Fork 4
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
PR to clean up the code #16
base: feature/particlenet
Are you sure you want to change the base?
Conversation
@@ -233,7 +231,7 @@ default: | |||
regionregex: .* | |||
apply_hf_cuts: True | |||
hf_pt_thresh: 80 | |||
region_without_dijet_cuts: False | |||
region_without_dijet_cuts: True |
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.
Do we want to keep this on by default? I think it's fine, I was just thinking it might introduce additional runtime + memory constraints. Especially removing the dijet cuts can increase the number of events passing quite a bit - hence making ParticleNet input dict occupy larger amounts of memory.
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.
let's keep this on hold. I think that we should check the DNN distributions without these cuts.
bucoffea/vbfhinv/definitions.py
Outdated
regions.update(tmp) | ||
to_add = ['met_sr', 'mindphijm'] | ||
to_remove = ['veto_muo', 'veto_tau', 'veto_ele', 'mindphijr', 'recoil'] | ||
regions.update(dict([(f"{region}_no_veto_all", add_lists(clean_lists(regions[region], to_remove),to_add)) for region in regions.keys() if region.startswith("sr_")])) |
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.
I love the one-liner, but it might be confusing for new people when looking at the code to understand what this does I think :) What do you think? Can you maybe at least make the for
loop explicit like the previous version? Or if you want to keep it this way, maybe a comment on top explaining what this does might be helpful.
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.
Done. Let me know if you want some more clarity more
if not os.path.exists(outdir): | ||
os.makedirs(outdir) | ||
outdir = pjoin('./output/',args.inpath.replace('..','').replace('/',''),'hf_estimate') | ||
dump_info(args, outdir) | ||
|
||
outrootpath = pjoin(outdir, 'vbfhinv_hf_estimate.root') |
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.
Quick question: If I understand this correctly, you're saving the file under output/{outtag}/hf_estimate
, and in the above script (where plot_data_mc
function is defined) I think you're reading the file from {outtag}/hf_estimate
. Did I get that right? Just wanted to check if it's working :)
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.
the folder should be the same in the end. maybe the naming convention could do a bit of polishing
…into particlenet Conflicts: bucoffea/vbfhinv/vbfhinvProcessor.py
No description provided.