-
Notifications
You must be signed in to change notification settings - Fork 121
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
Integrating systematics handling into nanoevents #529
Conversation
57e1e46
to
cd7741c
Compare
To finish and merge:
|
@nsmith- can you take a look? |
@nsmith- ping? |
@andrzejnovak too |
Neat! Two UX questions. Should there be a specific one way syst option rather than filling up or down with nominal for example? Could add_systematics also take directly 2XN array or is it only function such that it can be lazy? |
|
@nsmith- ping |
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 think it looks good, modulo users creating reference cycles in the user-supplied variation function.
I'm a bit confused how the UI looks w.r.t. looping over the weight systematic variations. I expected some function that multiplies all the nominal weights together, but it looks like here we only track up/down.
) | ||
|
||
self["__systematics__", f"__{name}__"] = awkward.virtual( | ||
varying_function, |
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'm worried it will be very hard for user-provided functions to not reference the array this virtual array is being attached to, and hence memory leaks.
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.
Even though it's not a complete awkward schema, should we put the systematics instead in a dictionary or functor that's returned to the user instead?
Usually the pattern as it is right now almost always asks that you're referencing/altering an array that's in the object you're adding a systematic to.
The corrections or variations themselves are likely not a part of the record array though.
the what
specification in add_systematic
is usually some value in the record array. The input variations are not.
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.
should we put the systematics instead in a dictionary or functor that's returned to the user instead
Can you sketch what this would look like for a user? I didn't quite follow
Sketching out ideas for rich/extensible/efficient systematics handling in nanoevents.
note / warning: This is not meant to suggest a buy-in framework for systematics, but rather to engineer a naming convention for extensions by which systematics can be added to an analysis. This naming convention is then assumed by statistics tools to create variations and ensembles expecting common data. What is here right now is attempting to get to the point where a schema can be defined (doing this beforehand is self defeating). So while it may look like I'm writing a deeply opt-in framework, what I am opting-in to is agreeing to an interface that we should put systematics in a field in an physics object or event called "systematics". I am not trying to extend nanoevents such that everyone has to use it forever to calculate systematics. I am trying to alter nanoevents to suit the naming convention we are solving for.
New methods class:
Systematic
. If an object has systematics associated to it then it should inherit from this.Also adds
systematics
subdir tonanoaod.methods
, there is a simple "Up/Down" systematics class there right now.Right now
NanoEvents
, and then the NanoAOD object classes.It may make more sense to tie it directly to NanoCollection so that everything gets it.
There is one special keyword so far
weight
which tells the systematics to appendweight_SystematicName
to the class instead of altering the value of a feature in the object. It's clunky right now but it gets the job done correctly.I'll try to keep
systematics.ipynb
working while I hack on this.Guiding ideas: