first infrastructure for BToJpsiK histmaking and tensor writing#675
first infrastructure for BToJpsiK histmaking and tensor writing#675pmlugato wants to merge 15 commits intoWMass:mainfrom
Conversation
davidwalter2
left a comment
davidwalter2
left a comment
There was a problem hiding this comment.
Looks good overall, there are a couple of more tricky cases due to the different nature of this analysis w.r.t. the existing ones, we should work on these a bit which may involve some structural changes.
General style comments which are specific for this repository and which we have adapted over time:
- command line arguments:
- use camelCase
- don't use "dest" keyword
- use common parser (in wremnants/utilities/parsing) if possible or stick to common format of option names etc.
- use snake_case for filenames and variable names if possible
- use logger instead of print
- remove comments that has been added for testing
| mpl.rcParams["figure.dpi"] = 300 | ||
|
|
||
| # TODO migrate butojpsik stuff to separate file and leave this untouched or generalize correctly... | ||
| BuToJpsiK = None |
There was a problem hiding this comment.
I suggest making this file a proper script and adding command line arguments with sth like --particle, type=str, choices=['muon', 'kaon', 'D'] to specify the particle type and use this option to steer the script.
Also add other options for output plots directory etc.
There was a problem hiding this comment.
done, see scripts/corrections/make_response_maps.py alongside wremnants/postprocessing/response_maps_utils.py
| ) | ||
| elif self.era == "2018": | ||
| from wremnants.datasets.datagroups2018 import ( | ||
| make_datagroups_2018 as make_datagroups, |
There was a problem hiding this comment.
The make_datagroups function does not generally correspond to an era. The idea is that make_datagroups configures a set of samples that are used for a specific analysis. For example the "make_datagroups_2016" is applicable to single and dimuon analyses such as W mass or alphaS but not necessarily restricted to 2016. The fact that we have called it "make_datagroups_2016" is a mistake and should be changed (not in this PR).
That said, it would be good that your PR follows a better logic. Maybe we can pass the make_datagroups function as an argument to the constructor instead?
|
Ok, David sent most of the smart comments I also wanted to make. I'll try to have a deeper look soon |
dimuon resonance histmaker, some bug fixes, better logic for new analysis flows concerning datagroups
davidwalter2
left a comment
davidwalter2
left a comment
There was a problem hiding this comment.
Only found a few smaller details, no commit needed if you can answer the question about why the abs is taken in this one case.
| parser.add_argument( | ||
| "--particleType", | ||
| type=str, | ||
| required=True, |
There was a problem hiding this comment.
make "muon" default and add "pion" as choice, looking ahead
| const double qop = charge / pt / std::cosh(eta); | ||
|
|
||
| const double dsigma = sigmarel_ * qop; | ||
| const double dsigma = std::abs(sigmarel_ * qop); |
| base_action = lambda x: hh.projectNoFlow( | ||
| collapseSyst(x[select]), h, overflow_ax | ||
| ) | ||
| # base_action = lambda x: collapseSyst(x[select]) |
3 participants
after refactoring will test pipelines of histmaking --> fitting and make sure things still all good, otherwise as a first push of the infrastructure it is ready enough