-
Notifications
You must be signed in to change notification settings - Fork 1
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
Version 2 of inference workflow #3
base: master
Are you sure you want to change the base?
Conversation
…ents, run_command.
…for any combine command of the form 'combine -m method -t -l'.
…d postfit shapes in a root file.
…lot of the contributions of the (sub-) processes.
…f all nuisances, collecting the results in a json file.
…tasks to perform exp and obs fits.
Small additions to the code to now also perform observed fits. An expected fit command that should work (using existing
And an observed fit command:
|
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 looks great, thanks @jomatthi! But I think there are still a few things to be ironed out. See below for some inline comments.
Here I only managed to to a partial review, but maybe one could address this set of comments first and then do another one for the second part. I also have a general comment:
- in many places you define properties called
<name>_inst
which just doreturn self.<name>
, which seems needlessly complex. In most cases you can useself.<name>
directly everywhere the property is needed. The_inst
would make sense for string-values parameters that actually represent a Python object, e.g. anod.Category
orod.Variable
. The value of<name>_inst
is then set by looking up the object called<name>
in the config (or a similar container).
…rectionlib based pileup treatment.
Thank you for the first set of comments and the valuable feedback, @dsavoiu. The last commits address the comments, more or less one commit per comment to hopefully help with reviewing. Further improvements using mixing are currently in development. |
The latest commit include a restructured version of the inference tasks, now using mixins to declare the used parameters for each tasks. Also, instead of using one task to produce expected and observed impacts, now two classes are defined: |
This PR implements a more thought-out version of the task structure of the inference workflow calculating the top tagging scale factors, impacts of the considered nuisance parameters, and pre- and postfit shapes. This is achieved by restructuring the inheritance and internal dependencies between the different blocks, and by introducing a
CombineBaseTask
and aFitMixin
class that incorporates common parameters needed by the other tasks. The same tasks introduced in PR #1 are also implemented in this version of the workflow. In addition, thetopsf.PlotShapesV2
task is implemented, taking the output root file oftops.PostFitShapesFromWorkspaceV2
. This is however not yet final and still to be polished and expanded to also plot the shapes of the different subprocesses contributions.To make sure only the shape information relevant for the wanted fit is used, the
topsf.CreateDatacards
task now requires awp_name
parameter to be set as the SF for the different working points are not to be fitted simultaneously.The
confirm_and_run.sh
script now also accepts and expands some predefined and often usedextra_params
.An example command that should trigger the entire workflow and result in impact plots for each of the SF: