New E/g HLT Pixel Matching : 92X #18541
Conversation
|
A new Pull Request was created by @Sam-Harper for master. It involves the following packages: DataFormats/EgammaReco @perrotta, @cmsbuild, @silviodonato, @fwyzard, @Martin-Grunewald, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison is ready Comparison Summary:
|
| <version ClassVersion="12" checksum="536071409"/> | ||
| <version ClassVersion="11" checksum="3831394066"/> | ||
| <version ClassVersion="10" checksum="534580602"/> | ||
| </class> | ||
| <ioread sourceClass="reco::ElectronSeed" version="[1-12]" targetClass="reco::ElectronSeed" source="float dRz2_; float dPhi2_; float dRz2Pos_; float dPhi2Pos_; float dRz1_; float dPhi1_; float dRz1Pos_; float dPhi1Pos_; unsigned char hitsMask_;" target="hitInfo_"> | ||
| <![CDATA[hitInfo_ = reco::ElectronSeed::createHitInfo(onfile.dPhi1Pos_,onfile.dPhi1_,onfile.dRz1Pos_,onfile.dRz1_,onfile.dPhi2Pos_,onfile.dPhi2_,onfile.dRz2Pos_,onfile.dRz2_,onfile.hitsMask_,newObj->recHits());]]> |
There was a problem hiding this comment.
I'm definitely not an expert of these dictionary generation directives. But starting from the error message repoted by the static analyzer
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18541/19556/llvm-analysis/report-b49c6b.html#EndPath
and looking for some documentation online (e.g.
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideCreatingNewProducts
and
https://root.cern.ch/root/html532/io/DataModelEvolution.html )
I think that "newObj" here is wrong, as it refers to the not yet created "target in-memory object", in this case the target "reco::ElectronSeed". Shouldn't it be "oldObj" here, instead?
Could please someone more knowledgeable than me have a look and suggest the actual fix?
There was a problem hiding this comment.
Hi Andrea, this was definitely something I struggled with.
However my understanding is that the new object is already created and this code is just setting hitInfo_ to the correct value.
it happens line 2986-2995:
vector<reco::ElectronSeed::PMVars>& hitInfo_ = *(vector<reco::ElectronSeed::PMVars>*)(target+offset_hitInfo_);
reco::ElectronSeed* newObj = (reco::ElectronSeed*)target;
hitInfo_ = reco::ElectronSeed::createHitInfo(onfile.dPhi1Pos_,onfile.dPhi1_,onfile.dRz1Pos_,onfile.dRz1_,onfile.dPhi2Pos_,onfile.dPhi2_,onfile.dRz2Pos_,onfile.dRz2_,onfile.hitsMask_,newObj->recHits());
I feel if target was pointing to an invalid location , hitInfo_ would also be bad. And newObj cant be null, this function is definitely being called and is using the newObj. Wouldnt it segfault in that case? If that data was corrupted, my hit mask and sub det ids would be messed up when reading back. I dont think they are:
https://sharper.web.cern.ch/sharper/cms/egamma/eleSeedValidation/readingOldSeedsV2/
Still you raise a worrying point and I'll keep digging to be 100% sure. I can probably do oldObj but its not straightforward at all (its not a reco::ElectronSeed*)
There was a problem hiding this comment.
@Dr15Jones please take a look.
Thank you.
There was a problem hiding this comment.
@pcanal We need some guidance on the IO Rule.
There was a problem hiding this comment.
I think the only guarantee on newObj is that it was default constructed. Therefore the call to newObj->recHits() might not return the RecHits but instead just give an empty container. It may be working now just because of a lucky ordering of how the exact version of ROOT you are calling happens to be filling the member data.
There was a problem hiding this comment.
I think the only guarantee on newObj is that it was default constructed.
Correct. In addition some of the I/O action may have already taken place (For example if the data member holding recHits() is listed before some of the source or target of this rule, it is likely to have been populated.
There was a problem hiding this comment.
thanks, I'll try to figure out a way around this then.
There was a problem hiding this comment.
hi @Sam-Harper @slava77 - which data tiers does this potentially affect?
|
Upon further consultation with Philippe, we determined that calling |
|
Ah excellent, thank you so much for this @Dr15Jones and @pcanal ! |
|
@slava77 : for the size concerns: I re-ran reco with: on /RelValTTbar_13/CMSSW_9_0_2-PU25ns_90X_upgrade2017_realistic_v20_HS_2017_resub-v1/GEN-SIM-DIGI-RAW Each job had a 1000 events current RECO: with this PR: So this surprised me, the new PR is actually smaller. So I looked into it. ~1/3 of these seeds are not ecalDriven which means I just have an empty vector and an int. While the old seeds 10 floats all set to infinity and two ints set to 0 and a char set to 0. And even when we have hits, I dont actually add much more data. So perhaps makes sense? Comments? |
|
On 5/4/17 9:01 AM, Sam-Harper wrote:
@slava77 <https://github.com/slava77> : for the size concerns:
I re-ran reco with:
step2 --mc --eventcontent RECOSIM,DQM --runUnscheduled --datatier
RECOSIM,DQMIO --conditions 90X_upgrade2017_realistic_v20 --step
RAW2DIGI,L1Reco,RECO,EI,DQM,VALIDATION --nThreads 4 --geometry
DB:Extended --era Run2_2017 --python_filename reco.py --no_exec
--customise Configuration/DataProcessing/Utils.addMonitoring
on
/RelValTTbar_13/CMSSW_9_0_2-PU25ns_90X_upgrade2017_realistic_v20_HS_2017_resub-v1/GEN-SIM-DIGI-RAW
Each job had a 1000 events
current RECO:
recoElectronSeeds_electronMergedSeeds__RECO. 8720.97 2867.36
recoElectronSeeds_electronMergedSeeds__RECO. 8063 2756.03
with this PR:
recoElectronSeeds_electronMergedSeeds__RECO. 7748.91 2351.35
recoElectronSeeds_electronMergedSeeds__RECO. 7912.43 2285.76
recoElectronSeeds_electronMergedSeeds__RECO. 8071.8 2385.3
So this surprised me, the new PR is actually smaller. So I looked into
it. ~1/3 of these seeds are not ecalDriven which means I just have an
empty vector and an int. While the old seeds 10 floats all set to
infinity and two ints set to 0 and a char set to 0. And even when we
have hits, I dont actually add much more data. So perhaps makes sense?
Comments?
Looks OK then.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18541 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbrPlSYQ9iCR0lLcVnh9T47meiBi-ks5r2fZGgaJpZM4NOJGu>.
|
|
@Sam-Harper |
|
+1 |
|
+1
|
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
Sam-Harper
changed the title
|
just want to say, thank you all very much for the speedy (and extremely helpful) suggestions and comments and checks to perform. Having seen how the other side of the ring does it, I'm very grateful that we have this process and experts such as you all to ensure our code is of the standard required. |
|
On 5/5/17 7:35 AM, David Lange wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In DataFormats/EgammaReco/src/classes_def.xml
<#18541 (comment)>:
> <version ClassVersion="12" checksum="536071409"/>
<version ClassVersion="11" checksum="3831394066"/>
<version ClassVersion="10" checksum="534580602"/>
</class>
+ <ioread sourceClass="reco::ElectronSeed" version="[1-12]" targetClass="reco::ElectronSeed" source="float dRz2_; float dPhi2_; float dRz2Pos_; float dPhi2Pos_; float dRz1_; float dPhi1_; float dRz1Pos_; float dPhi1Pos_; unsigned char hitsMask_;" target="hitInfo_">
+ <![CDATA[hitInfo_ = reco::ElectronSeed::createHitInfo(onfile.dPhi1Pos_,onfile.dPhi1_,onfile.dRz1Pos_,onfile.dRz1_,onfile.dPhi2Pos_,onfile.dPhi2_,onfile.dRz2Pos_,onfile.dRz2_,onfile.hitsMask_,newObj->recHits());]]>
hi @Sam-Harper <https://github.com/Sam-Harper> @slava77
<https://github.com/slava77> - which data tiers does this potentially
affect?
RECO and its supersets (FEVT varieties) as well as probably some user/alca.
These are not in AOD or miniAOD.
ElectronSeedRef is also in GsfElectronCore and PhotonCore, which are in
[mini]AOD, but it's just a ref.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18541 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbpL6dcjSYAHhjofZBxM4rkQxQhNKks5r2zO5gaJpZM4NOJGu>.
|
|
+1
… On May 5, 2017, at 5:30 PM, Slava Krutelyov ***@***.***> wrote:
On 5/5/17 7:35 AM, David Lange wrote:
> ***@***.**** commented on this pull request.
>
> ------------------------------------------------------------------------
>
> In DataFormats/EgammaReco/src/classes_def.xml
> <#18541 (comment)>:
>
>> <version ClassVersion="12" checksum="536071409"/>
> <version ClassVersion="11" checksum="3831394066"/>
> <version ClassVersion="10" checksum="534580602"/>
> </class>
> + <ioread sourceClass="reco::ElectronSeed" version="[1-12]" targetClass="reco::ElectronSeed" source="float dRz2_; float dPhi2_; float dRz2Pos_; float dPhi2Pos_; float dRz1_; float dPhi1_; float dRz1Pos_; float dPhi1Pos_; unsigned char hitsMask_;" target="hitInfo_">
> + <![CDATA[hitInfo_ = reco::ElectronSeed::createHitInfo(onfile.dPhi1Pos_,onfile.dPhi1_,onfile.dRz1Pos_,onfile.dRz1_,onfile.dPhi2Pos_,onfile.dPhi2_,onfile.dRz2Pos_,onfile.dRz2_,onfile.hitsMask_,newObj->recHits());]]>
>
> hi @Sam-Harper <https://github.com/Sam-Harper> @slava77
> <https://github.com/slava77> - which data tiers does this potentially
> affect?
>
RECO and its supersets (FEVT varieties) as well as probably some user/alca.
These are not in AOD or miniAOD.
ElectronSeedRef is also in GsfElectronCore and PhotonCore, which are in
[mini]AOD, but it's just a ref.
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#18541 (comment)>, or
> mute the thread
> <https://github.com/notifications/unsubscribe-auth/AEdcbpL6dcjSYAHhjofZBxM4rkQxQhNKks5r2zO5gaJpZM4NOJGu>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Here is the 91X flavour of #18527 where the changes are explained in more detail
This allows us to have a variable number of pixel hit matches at the HLT, specifically to allow us to go to triplets & doublets rather than just triplets. It also upgrades us to the new tracking framework for seeding.
However for RECO, no changes are expected currently. So there should not be a single change observed from this PR.