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
HCAL: adding Phase 1 HBX/HEX channels to CalibDetId and ASCII conditions processing #26432
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26432/9212
|
A new Pull Request was created by @abdoulline (Salavat Abdullin) for master. It involves the following packages: CalibFormats/HcalObjects @civanch, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @mdhildreth, @pohsun 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 job queued. |
Comparison is ready Comparison Summary:
|
flavorName="HBX"; | ||
setField (1, calibId.ieta()); | ||
setField (2, calibId.iphi()); | ||
setField (3, calibId.depth()); |
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.
Is there a reason that you are assigning a depth here (and in the HEX channels) rather than using a dummy value of -999 as in the HOX channels in L145? The depth value isn't meaningful as these channels do not read out any scintillator.
The attributes/coordinates of these channels have been discussed in a
dedicated thread with Richard Kellog (HCAL master of emap) Maria Toms (OPS
convener), Ted Laird (commissioning and PM deputy) et al.
And Richard explicitly said/agreed (HxX + ieta + iphi + depth) make sense to him.
Let me re-iterate on this, including you, Chris.
…On Thu, 11 Apr 2019, christopheralanwest wrote:
@christopheralanwest commented on this pull request.
___________________________________________________________________________________________________________
In CalibFormats/HcalObjects/src/HcalText2DetIdConverter.cc:
> @@ -158,7 +158,17 @@ bool HcalText2DetIdConverter::init (DetId fId) {
setField (1, calibId.rm());
setField (2, calibId.fiber());
setField (3, calibId.channel());
- }
+ } else if ( calibId.calibFlavor()==HcalCalibDetId::HBX) {
+ flavorName="HBX";
+ setField (1, calibId.ieta());
+ setField (2, calibId.iphi());
+ setField (3, calibId.depth());
Is there a reason that you are assigning a depth here (and in the HEX channels) rather than using a dummy
value of -999 as in the HOX channels in L145? The depth value isn't meaningful as these channels do not
read out any scintillator.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02k9cvrqWUdxvUHFM1iTXJQM-eWQYks5vf2OtgaJpZM4couZ6.gif]
|
@christopheralanwest |
Pull request #26432 was updated. @civanch, @christopheralanwest, @tocheng, @cmsbuild, @franzoni, @mdhildreth, @pohsun can you please check and sign again. |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
int ieta=getField(1); | ||
int iphi=getField(2); | ||
mId = HcalCalibDetId (ieta,iphi); | ||
mId = (flavorName=="HOX")? |
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.
Salavat, This construction seems not to be optimal: why the same string is compared twice? It is possible to check once flavorName=="HOX" , flavorName=="HBX"...
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.
@civanch @abdoulline I agree this logic might made a bit simpler by splitting the three new cases into separate actions...
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.
@fabiocos Fabio, I hope it doesn't prevent the PR from being merged as is?
HCAL DPG is waiting for it to proceed with updating emap (extended for HBX/HEX channels).
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.
@abdoulline I will integrate for this IB, not the cleanest logic to my mind, but if it is working correctly fine, it could be cleaned in some further pass
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.
@fabiocos it behaves/works correctly, the way it coded can be made more explicit/transparent next time.
Vladimir, I might have misunderstood your comment.
I just wanted 1 line here (not "case" or "if-else") for this _standalone_
ASCII conditions parser (which is not run in any regular workflow).
BTW, doesn't compiler optimization take care of things like this?
…On Sun, 14 Apr 2019, Vladimir Ivantchenko wrote:
@civanch commented on this pull request.
_____________________________________________________________________________________________________________________________________________________________
In CalibFormats/HcalObjects/src/HcalText2DetIdConverter.cc:
> int ieta=getField(1);
int iphi=getField(2);
- mId = HcalCalibDetId (ieta,iphi);
+ mId = (flavorName=="HOX")?
This construction is not optimal: why the same string is compared twice?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.[AEx02nldSDRxsaxwUJuoqonj8Cv_DlFyks5vgybIgaJpZM4couZ6.gif]
|
+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 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 |
PR description:
Preparatory modifications/extensions of HcalCalibDetId and ASCII conditions interpreter (reading/dumping) for handling Phase 1 HB and HE monitoring channels.
PR validation:
runTheMatrix is OK.
simple sanity check: reading existing emap (from txt) with 2 newly added HBX and HEX channels shows expected behavior, with properly interpreted (packed/unpacked=set/got) attributes/fields.
Here is a diff of the dumped emap wrt the input one (via CMSSW HCAL DB functionality, means txt input is read and parsed, a transient emap object is created and then dumped ):
64c64
4E2CC01C 3 3 t 24 2 5 2 HBX 16 3 -999
->
4E240A03 3 3 t 24 2 5 2 HBX 16 3 -999
73c73
4E2EC01C 3 3 t 24 2 8 2 HEX 16 3 -999
->
4E240A01 3 3 t 24 2 8 2 HEX 16 3 -999
All other ~27k HCAL channels of various types (phys.channels, trig.primitives, several sorts of calib. channels) stays unchanged in this test.
NB: no any changes expected in any workflow at the moment.
When emap will be modified in GT and HCAL DQM (to cope with new channels) will be updated, then some new DQM histos will show these channels in data.