-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update the ADC->fC table, simplify the charge reconstruction algorith… #22974
Conversation
…m, enable the MET filter
The code-checks are being triggered in jenkins. |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22974/4357 Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22974/4357/git-diff.patch You can run |
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22974/4359 |
A new Pull Request was created by @jkunkle for master. It involves the following packages: RecoMET/METFilters @perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
7272,7597,7922,8247,8572,8897,9222,9547,10197,10847,11497,12147, | ||
12797,13447,14097,15072,16047,17022,17997,19297,20597,21897,23522,25147}; | ||
//adc2fCHF = std::vector<float> {-3,-0.4,2.2,4.8,7.4,10,12.6,15.2,17.8,20.4,23,25.6,28.2,30.8,33.4, | ||
// 36,41.2,46.4,51.6,56.8,62,67.2,73,80.8,88.6,96.4,104,114.4,124.8,135, |
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 this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
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.
Indeed, I neglected to remove it. I've pushed the change
@cmsbuild please test |
The tests are being triggered in jenkins. |
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22974/4790 |
I made the suggested code update. The testing prescription is not working just because the CMSSW version is outdated, I repeat the instructions below with updated CMSSW version
|
please test |
The tests are being triggered in jenkins. |
@jkunkle : in your recipe, how can a 10_1_X GT work in 10_2_X ? |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
Evidently you never tried the recipe in #22974 (comment), which is rather disappointing because I am supposed to have more interesting things to do rather than debugging your private test code.
While running that test, in the reco step I repeatedly get the following warnings in outputs (e.g.):
They may be originated by the filter code (not necessarily from these latest updates: but you have some related flag in your test, even if the warning itself comes from the Tracker ClusterSummary [1]), and they could become rather annoying if and when it will be switched on. But maybe they just derive from a loose threshold for warnings which was set in your test. [1] http://cmslxr.fnal.gov/source/DataFormats/TrackerCommon/src/ClusterSummary.cc#0047 |
I certainly tried the the testing procedure that I gave, and it produces the expected results as-is. If you observed some failure in the procedure that I gave (which I did not see, since it worked for me) then it would be good to know what that is. Otherwise it is up to you if you want to make unneeded changes to the testing procedure. I used the GT that was consistent with the conditions used when the data sample was recorded. It is never clear to me what the 'correct' prescription for applying the GT is (if you know of some clear prescription please let me know), but I kept the GT associated to the conditions of that run. I don't see any need to change the trackedness of the configuration for the test, and I don't see any problem with the source file name, again, it works for me as-is. The warnings that you see are not related to this PR. This can be checked by simply not merging this pull request and running the cmsDriver commands. My aim was to run the 'nominal' data processing chain by using the cmsDriver commands. Perhaps I'm not running the exact correct cmsDriver command which exposes these warnings. |
+1
|
+1 |
merge |
Updates to enable the HCAL laser monitoring. With this change, I take the opportunity to make a simplification of the code which performs the laser monitoring charge reconstruction. We have implemented the lasermon system in firmware rather than hardware. This means that we can remove the complicated matching algorithm because the firmware guarantees fixed latency between channels (the hardware implementation did not). The relevant changes are,
-RecoMET/METFilters/plugins/HcalLaserEventFilter.cc - enable the laser filtering and update the threshold to 5000
The above changes have been tested with a recent global run for consistency against the old method and that the changes propagate to the Met filter
@deguio @mariadalfonso