Add the HF Filters in Heavy Ions miniAOD #30810
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30810/17145
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
| @@ -135,6 +135,7 @@ | |||
|
|
|||
| _pp_on_AA_extraCommands = [ | |||
| 'keep patPackedCandidates_hiPixelTracks_*_*', | |||
| 'keep ints_hihffilter_*_*', | |||
There was a problem hiding this comment.
hiHFfilter may be a bit more readable
There was a problem hiding this comment.
implemented in the commit b0ccac9
There was a problem hiding this comment.
thanks for the PR @denerslemos, please see the attached comments to be addressed. Also, please do the code checks as described here: https://cms-sw.github.io/PRWorkflow.html
| #include <cmath> | ||
| #include "TMath.h" | ||
|
|
||
| class HIhfFilter_miniAOD : public edm::stream::EDProducer<> { |
There was a problem hiding this comment.
Consider a name that is closer to the existing producers in CMSSW. _miniAOD should not be included, in addition, the names are typically in CamelCase. Looking at the examples in RecoHI/HiCentralityAlgos/plugins, how about something like HiHFFilterProducer.
There was a problem hiding this comment.
implemented in the commit b0ccac9
| HIhfFilter_miniAOD::HIhfFilter_miniAOD(const edm::ParameterSet& iConfig): | ||
| srcTowers_(consumes<CaloTowerCollection>(iConfig.getParameter<edm::InputTag>("srcTowers"))) | ||
| { | ||
| produces<std::vector<int>>("HIhfFilters"); |
There was a problem hiding this comment.
Saving the filter bits as a hardcoded vector may be difficult and error-prone to use in the future by others. Instead, create and save a struct like
struct HFFilterResult {
float numMinHFTowers2;
float numMinHFTowers3;
float numMinHFTowers4;
float numMinHFTowers5;
}
produces<HFFilterResult>("hiHFFilters")There was a problem hiding this comment.
I tried to implement that struct like instead of the vectors, using this way that you point to me, however, when I include produces("hiHFFilters"), I got this error message:
----- Begin Fatal Exception 27-Jul-2020 19:33:06 CEST-----------------------
An exception of category 'DictionaryNotFound' occurred while
[0] Constructing the EventProcessor
[1] Constructing module: class=HiHFFilterProducer label='hiHFFilterProducer'
[2] Calling ProductRegistryHelper::addToRegistry, checking dictionaries for produced types
Exception Message:
No data dictionary found for the following classes:
HiHFFilterProducer::HFFilterResult
edm::WrapperHiHFFilterProducer::HFFilterResult
Most likely each dictionary was never generated, but it may
be that it was generated in the wrong package. Please add
(or move) the specification '' to
the appropriate classes_def.xml file along with any other
information needed there. For example, if this class has any
transient members, you need to specify them in classes_def.xml.
Also include the class header in classes.h
A type listed above might or might not be the same as a
type declared by a producer module with the function 'produces'.
Instead it might be the type of a data member, base class,
wrapped type, or other object needed by a produced type. Below
is some additional information which lists the types declared
to be produced by a producer module that are associated with
the types whose dictionaries were not found:
HiHFFilterProducer::HFFilterResult
I tried to declare the HFFilterResult in private, in public and even out of the class, all of than with the same error. I still did not give up, and I am trying to use some ideas from this twiki:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideCreatingNewProducts
Do you have another suggestion or a simpler way to do this?
| } | ||
|
|
||
|
|
||
| HIhfFilter_miniAOD::~HIhfFilter_miniAOD() |
There was a problem hiding this comment.
Empty destructors are not needed and should be removed.
There was a problem hiding this comment.
implemented in the commit b0ccac9
| using namespace std; | ||
|
|
||
| Handle<CaloTowerCollection> towers; | ||
| iEvent.getByToken(srcTowers_, towers); |
There was a problem hiding this comment.
getByToken is disfavored, use auto const& towers = event.get(srcTowers_);
There was a problem hiding this comment.
implemented in the commit b0ccac9
|
|
||
| for( unsigned int towerIndx = 0; towerIndx<towers->size(); ++towerIndx){ | ||
| const CaloTower & tower = (*towers)[ towerIndx ]; | ||
| if(tower.et() >= 0.0){ |
There was a problem hiding this comment.
How about:
const auto energy = tower.energy();
const auto eta = tower.eta();
const bool eta_plus = (eta>3.0) && (eta<6.0);
const bool eta_minus = (eta<-3.0) && (eta>-6.0);
if (eta_plus) {
if (energy >= 2.0) nTowersTh2HFplus_ += 1;
if (energy >= 3.0) nTowersTh3HFplus_ += 1;
...
} else if (eta_minus) {
if (energy >= 2.0) nTowersTh2HFminus_ += 1;
if (energy >= 3.0) nTowersTh3HFminus_ += 1;
}There was a problem hiding this comment.
implemented in the commit b0ccac9
| } | ||
|
|
||
| void | ||
| HIhfFilter_miniAOD::beginStream(edm::StreamID) |
There was a problem hiding this comment.
I think these empty beginStream, endStream functions can be removed.
There was a problem hiding this comment.
implemented in the commit b0ccac9
| } | ||
| } | ||
|
|
||
| (*HIhfFiltersOut)[0] = TMath::Min(nTowersTh2HFplus_,nTowersTh2HFminus_); |
There was a problem hiding this comment.
implemented in the commit b0ccac9
| Handle<CaloTowerCollection> towers; | ||
| iEvent.getByToken(srcTowers_, towers); | ||
|
|
||
| int nTowersTh2HFplus_ =0; |
There was a problem hiding this comment.
per https://cms-sw.github.io/cms_coding_rules.html
Do not use “_” as first character, except for user-defined suffixes (used in user-defined literals). Only use it as the last character for class data member names, not local variable names."
local variables should not end with an underscore (only class data members).
There was a problem hiding this comment.
implemented in the commit b0ccac9
|
kind remind @denerslemos ( |
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30810/17343
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30810/17344
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30810/17345
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30810/17346
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
Hi everyone, sorry for the delay. I have tried to correct this PR since the beginning of the comments, however the code-checks commands are not working for me. Most of the comments are already implemented, but the code-checks does not work for me. Can I close the PR and start a new one from the scratch? This new PR already will have implemented the comments from this one. |
yes, sure. Please add a link to #30810 in the new PR in order to get track of the old comments |
Dear All,
This PR is made to include the HF event filters in the Heavy Ions miniAOD workflow. This is expected to be included only for the eras "pp_on_AA_2018" and "pp_on_PbPb_run3". Since the towerMaker collection is not saved in miniAOD this filters need be stored in the miniAOD content. To include that filters, a EDProducer was created to run on PAT and store a vector of integers with size 4 which each position in this vector has the minimum number of towers above some threshold between the two HF sides (TMath::Min(nTowerHF+,nTowerHF-)), which means:
Tests comparing miniAOD and AOD was made, and the result of the application of the filters are exactly the same in both. For example, using hfCoincFilter2Th4 filter (which requires 2 HF+/- towers above 4 GeV) both AOD and miniAOD has 5723 passed events and 1237 failed events (total events: 6960 for MB sample). Also, the number of tracks removed for both AOD (generalTracks with highPurity and pT > 0.5 GeV) and miniAOD (packedPFcandidates) was exactly the same: 142. In this studies the vector increases the size of the miniAOD in ~6 bytes per event. You can find more details of this slides:
https://www.dropbox.com/s/susuzaanbk3jb3h/HIHFFilter.pdf?dl=0
The studies below was made in 103X, and was also tested in 112X using "runTheMatrix.py -l limited -i all --ibeos", in special workflow 159. The versions 103X and 112X returns exactly the same results on the tests.
@mandrenguyen @YeonjuGo @pjwinnetou
Best,
Dener Lemos