Phase2-hgx111 Modify reco geometry/topology for post TDR HGCAL #23211
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23211/4670 |
|
A new Pull Request was created by @bsunanda for master. It involves the following packages: DataFormats/ForwardDetId @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild Please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| if (type == 0) { | ||
| subdet_ = HGCEE; | ||
| det_ = (int)HGCEE; |
There was a problem hiding this comment.
@bsunanda - this is potentially confusing. The integer type picked up from configuration file is not equal the integer in enumeration. It is unsafe because these enums defined in two different places:
enum ForwardSubdetector { ForwardEmpty=0, FastTime=1, BHM=2, HGCEE=3, HGCHEF=4,
HGCHEB=5, HGCHET=6, HGCTrigger=7 };
enum Detector {Tracker=1, Muon=2, Ecal=3, Hcal=4, Calo=5, Forward=6,
VeryForward=7, HGCalEE=8, HGCalHSi=9, HGCalHSc=10,
HGCalTrigger=11};
The logic as implemented:
if 0 then 3
else if 1 then 4
else if 2 then 8
else if 2 then 9 // which will never happen
else 10
IMHO, it would be safer to use strongly typed enum class.
| @@ -73,6 +73,9 @@ class FlatTrd : public CaloCellGeometry { | |||
|
|
|||
| void setPosition ( const GlobalPoint& p ) { m_global = p; setRefPoint(p); } | |||
|
|
|||
| static const unsigned int ncorner_ = 8; | |||
| static constexpr unsigned int ncornerBy2_ = ncorner_/2; | |||
| int HGCalDDDConstants::getLayer(double z, bool reco) const { | ||
| int lay = (int)(hgpar_->zLayerHex_.size()); | ||
| double zz = std::abs(z); | ||
| for (unsigned int k=1; k<hgpar_->zLayerHex_.size(); ++k) { |
There was a problem hiding this comment.
I would like to check if z lies below 0.5*(z_i+z_i+1). I don't think std::find_if will not give that -it tries to equate or not-equate. Am I wrong on this?
There was a problem hiding this comment.
You can use an arbitrary lambda in find_if(), but since this comparison requires two elements from the vector, I don't think find_if() will work.
There was a problem hiding this comment.
You can use an outer variable to do the indexing, capturing it into the lambda.
There was a problem hiding this comment.
Ah, I see, a construction like this: https://stackoverflow.com/questions/3752019/how-to-get-the-index-of-a-value-in-a-vector-using-for-each
So the code here might be modified as follows (haven't tried to compile it):
unsigned k = 0;
auto it = std::find_if(hgpar_->zLayerHex_.begin()+1,hgpar_->zLayerHex_.end(),[&k,&zz,&hgpar_->zLayerHex_](double zLayer){ ++k; return zz < 0.5*(zLayerHex_[k-1]+zLayerHex_[k]); });
lay = k;
| int cellV) const { | ||
| int indx = HGCalWaferIndex::waferIndex(layer,modU,modV); | ||
| auto itr = hgpar_->typesInLayers_.find(indx); | ||
| bool ok(false); |
There was a problem hiding this comment.
perhaps, renaming it to:
bool result(false);
would be less confusing when returning ok == false
|
|
||
| bool HGCalDDDConstants::isValidTrap(int layer, int ieta, int iphi) const { | ||
| std::pair<int,float> indx = getIndex(layer,true); | ||
| bool ok(false); |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@bsunanda there are some minor changes in clusters/PFCandidates and downstream objects for Phase2 workflows. It's not clear to me what would cause that in this PR. Do you know why it happens? |
|
There was a bug in the GeomID in HGCalDetId class which is fixed. It might be the cause for the changes. I found this bug while making geometry ID's of the new description |
|
Does this bug just affect decoding the IDs, or did it also enter the encoding of IDs? I would like to understand if this will cause an incompatibility with reading old files. |
|
It happens when one converts DetId to GeomId. So old data will be fine and geometry is always recreated.
…________________________________
From: Kevin Pedro [notifications@github.com]
Sent: 18 May 2018 18:34
To: cms-sw/cmssw
Cc: Sunanda Banerjee; Mention
Subject: Re: [cms-sw/cmssw] Phase2-hgx111 Modify reco geometry/topology for post TDR HGCAL (#23211)
Does this bug just affect decoding the IDs, or did it also enter the encoding of IDs? I would like to understand if this will cause an incompatibility with reading old files.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#23211 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEzMupksUeZHfO20lA7Hax0Hd32KMmCuks5tzvf7gaJpZM4UAZgu>.
|
|
+1 |
1 similar comment
|
+1 |
cmsbuild
added
fully-signed
geometry-approved
simulation-approved
and removed
geometry-pending
pending-signatures
simulation-pending
labels
|
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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
Now Reco Geometry/topology can handle the new geometry. No provision of finding neighbours yet