-
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
Improve various Patatrack Kernels #35598
Conversation
enable gpu |
@cmsbuild , please test |
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35598/25860
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
@cmsbuild , please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35598/25861
|
A new Pull Request was created by @VinInn (Vincenzo Innocente) for master. It involves the following packages:
@malbouis, @yuanchao, @ggovi, @slava77, @jpata, @tvami, @francescobrivio can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@@ -38,7 +38,7 @@ void SiPixelGainCalibrationForHLTService::calibrate( | |||
if (isDeadColumn | isNoisyColumn) | |||
electron[i++] = 0; | |||
else { | |||
float vcal = di->adc() * gain - pedestal * gain; | |||
float vcal = float(di->adc()) * gain - pedestal * gain; | |||
// float vcal = (di->adc() - DBpedestal) * DBgain; |
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.
please remove the commented out code, or explain why it might be needed, thanks Vincenzo!
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.
It was there before.
In principle maybe faster and more precise, BUT I assume the calibration is done with the above so better to keep that.
Anyhow trivial to remove.
@@ -28,7 +28,7 @@ void SiPixelGainCalibrationServiceBase::calibrate( | |||
else { | |||
float DBgain = getGain(detID, col, row); | |||
float DBpedestal = getPedestal(detID, col, row) * DBgain; | |||
float vcal = di->adc() * DBgain - DBpedestal; | |||
float vcal = float(di->adc()) * DBgain - DBpedestal; | |||
// float vcal = (di->adc() - DBpedestal) * DBgain; |
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.
Same comment as above regarding commented code
-1 Failed Tests: RelVals-GPU RelVals-GPU
Comparison SummarySummary:
|
Tried to reproduce runTheMatrix crashes by itself though
|
same in CMSSW_12_1_X_2021-10-09-1100 |
ok. reproduced on MC using a hlt menu of my own... |
@cmsbuild , please test |
oops messed up the merge |
superseded by #35835 |
Pull request #35598 was updated. @SiewYan, @gouskos, @bbilin, @wajidalikhan, @pmandrik, @ianna, @Saptaparna, @Martin-Grunewald, @ggovi, @alberto-sanchez, @santocch, @cecilecaillol, @perrotta, @civanch, @yuanchao, @makortel, @ahmad3213, @missirol, @agrohsje, @fwyzard, @GurpreetSinghChahal, @smorovic, @davidlange6, @smuzaffar, @Dr15Jones, @cvuosalo, @emanueleusai, @mdhildreth, @AdrianoDee, @jfernan2, @kskovpen, @slava77, @jpata, @qliphy, @fabiocos, @pbo0, @francescobrivio, @malbouis, @mkirsano, @jordan-martins, @rekovic, @emeschi, @alja, @srimanob, @fgolf, @mariadalfonso, @tvami, @rvenditti can you please check and sign again. |
Hi @VinInn I think you need to rebase now to the latest IBs |
ok, closing is also an option :) |
In any case the thread here was long and confusing due the too many spurious errors |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-80c871/19919/summary.html GPU Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
Comparison SummarySummary:
|
Simplify and improve the logic of various Kernels.
Some maths have been sync with CPU version.
Fixed and improved the statistics collection and printing (off by default).
Technical.
No regression observed: math has changed so some regression cannot be excluded (even in CPU wf).