-
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
Stricter blinding cut #206
Conversation
Do not allow blinding cut to be modified by init(kwargs) Rename "blinding_cut" to "unblinding_selection" Automatically unblind MC Make MC flag global (not just in minitrees) Reduce code redundancy in e-lifetime a bit lint fixes
Do not allow blinding cut to be modified by init(kwargs) Rename "blinding_cut" to "unblinding_selection" Automatically unblind MC Make MC flag global (not just in minitrees) Reduce code redundancy in e-lifetime a bit lint fixes
(though not sure what to do with other eval above)
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.
What about support for different SR0/SR1 cuts? Or is this not needed?
Otherwise I like it in general. I hesitate to say this but I really think there's no way to 'accidentally unblind' now. Though I thought that before...
Added the SR0 plot here. Quite similar to SR1, so I don't think using the same lines for SR0 will bias us. |
OK. Might want a second or third review also since important. |
hax/__init__.py
Outdated
unblinding_config = {} | ||
|
||
# Flag for whether you are working with MC (True) or not | ||
mc_data = False |
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.
Could you clarify what this global mc_data is doing? It looks like this is a run-level property, but it's stored as a package global. You might get some interesting behaviour if you're doing real data / MC comparison studies. Could the treemakers that need to know whether they are in MC land (or maybe event the treemaker base class) call hax.runs.is_mc instead?
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.
Remnant from confusion during development. Fixed in 1154d04.
hax/__init__.py
Outdated
unblinding_config = {} | ||
section_to_use = 'UNBLINDING' | ||
for key, value in unblinding_configp[section_to_use].items(): | ||
unblinding_config[key] = ast.literal_eval(value) |
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.
Since you're only storing two settings in unblinding.ini, and the unblinding.ini filename can't be overriden by the user, would it be easier to make unblinding.ini just unblinding.py? Then add unblinding here https://github.com/XENON1T/hax/blob/master/hax/__init__.py#L7, and you can access the settings as hax.unblinding.unblinding_selection
and hax.unblinding.blind_from_run
when you need them in hax.runs.
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.
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.
Yes, it should work just as well. I haven't looked at #199 in detail, but maybe you want to put all settings in a settings
dictionary inside unblinding.py
if you have to do a lot of dynamic key access (getattr(hax.unblinding, key)
isn't so great). You could even move is_blind
into the file as well to have all blinding logic together.
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.
hax/corrections_handler.py
Outdated
|
||
except Exception as e: | ||
print(e) | ||
return 1. |
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.
If there is no electron lifetime available, are you sure you want to produce a correction minitree?
The generic except catching with a default of 1 instead of NaN might cause some fun later on. If someone puts a syntax error in get_electron_lifetime
, everything will run happily but we'll get no electron lifetime corrections. Hopefully someone is checking the logs and they'll see there was a syntaxerror somewhere (but since it's just a print they also don't know where).
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.
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 copy-pasted code from @jpienaar13 who had added this condition for MC I think (or at least that's what I thought when I copy-pasted). But we could remove it as far as I'm concerned.
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.
OK raising the exception instead in e3525fd
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.
@coderdj Pretty sure I didnt add the e-lifetime=1 option. in fact i believe git blame says you did.
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.
Yeah now that I look at it I did. I don't know why I did.
But the other part is true. When I did the refactor of the corrections stuff I also noticed it was weird and copy-pasted it thinking you added it for MC stuff (without checking if that was the case).
Anyway, it can be removed.
@@ -307,6 +307,16 @@ def get_run_start(run_id): | |||
# return np.datetime64('2017-06-13T18:17:43.000000000') | |||
|
|||
|
|||
def is_mc(run_id): |
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.
Glad you cleaned up this code duplication!
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.
Looks good, I can understand having the blinding_cut option so easily overridable in hax.init may be too tempting. Factoring the blinding settings out to a separate file also makes it easier to track the history of the blinding cut.
I made a few suggestions for avoiding the extra config parsing and the new package globals. I think they're easy and might save time in the long run, but see for yourself.
also forgot logger in unblinding.py
hax/__init__.py
Outdated
@@ -1,11 +1,12 @@ | |||
import logging | |||
import os | |||
import ast |
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.
This is now unused, though I suppose you could replace the eval of the main config with this (currently it has a "slightly" less secure eval with the os
module exposed (which is also no longer used))
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.
Just removed in 6ad82f6 (since I couldn't easily just swap eval
with ast.literal_eval
).
hax/corrections_handler.py
Outdated
elifetime = self.get_misc_correction("mc_electron_lifetime_liquid", run_number) | ||
|
||
else: | ||
try: |
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 can remove the whole try-except now, it just raises every exception it encounters (which happens anyway)
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.
Fixed in 01169a1
* Sideband unblinding and update blinding doc * More readable blinding, and also unblind S2<200 * Remove #199 comment for High-E below NR (already unblinded before this PR) * Adapt to stricter blinding implementation (#206) * Unblind below S2 = 200 pe (threshold) Following #208 * Remove redundant s2 < 200 * Remove unnecessary s2>200 at high energy unblindin * Remove obsolete comment
Implement a more stricter blinding cut (as requested here):
Move definition from
hax.ini
into separateunblinding.ini
file (the new name should be more scary to edit).Move cut string from
hax.config
tohax.unblinding_config
such that it cannot be modified withhax.init(kwargs)
as before.a. Thus, Warn on blinding cut customization #204 becomes obsolete since it's not allowed at all here. @JelleAalbers what other legitimate uses were you thinking?
Rename
blinding_cut
tounblinding_selection
to break backwards compatibility of any rogue scripts (though should already be taken care of by 2. above)Automatically unblind MC (previously, manually using
kwargs
in secret MC notebooks and laxer).a. Moved
mc_data
variable fromminitrees.py
tohax
global variable to reduce code duplication.b. Reduced code duplication for MC checks and e-lifetime.
Henceforth, the only way to unblind is to submit a PR modifying
unblinding.ini
(or still a few other previous malicious ways, but please don't).