Skip to content
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

HBHE: mahi speed up #25982

Merged
merged 9 commits into from Mar 12, 2019
Merged

HBHE: mahi speed up #25982

merged 9 commits into from Mar 12, 2019

Conversation

mariadalfonso
Copy link
Contributor

@mariadalfonso mariadalfonso commented Feb 20, 2019

Code cleanup, followed from V.Innocente's suggestions, resulted in a 10% speed improvement.
Tested on data 2018-RunB, see igproof below (at commit dfc08db ).

No changes are expected in the HCAL quantities, neither energy/time.

                    • OLD - - - - - - - - - - - - - - - - - - - -
            1.5  .........       1.77 / 2.13         SimpleHBHEPhase1Algo::reconstruct(HBHEChannelInfo const&, HcalRecoParam const*, HcalCalibrations const&, bool) [67]
[76]        1.5       1.77       0.07 / 1.69       MahiFit::phase1Apply(HBHEChannelInfo const&, float&, float&, bool&, float&) const
            1.3  .........       1.62 / 1.62         MahiFit::doFit(std::array<float, 3ul>&, int) const [85]
            0.1  .........       0.07 / 0.07         MahiFit::resetWorkspace() const [786]
            1.3  .........       1.62 / 1.77         MahiFit::phase1Apply(HBHEChannelInfo const&, float&, float&, bool&, float&) const [76]
[85]        1.3       1.62       0.03 / 1.59       MahiFit::doFit(std::array<float, 3ul>&, int) const
            1.0  .........       1.23 / 1.23         MahiFit::minimize() const [120]
            0.2  .........       0.20 / 0.20         MahiFit::updatePulseShape(double, Eigen::Matrix<double, -1, 1, 0, 19, 1>&, Eigen::Matrix<double, -1, 1, 0, 19, 1>&, Eigen::Matrix<double, -1, -1, 0, 19, 19>&) const [426]
            0.1  .........       0.16 / 0.16         MahiFit::calculateArrivalTime() const [489]
                    • NEW - - - - - - - - - - - - - - - - - - - -
            1.3  .........       1.56 / 1.95         SimpleHBHEPhase1Algo::reconstruct(HBHEChannelInfo const&, HcalRecoParam const*, HcalCalibrations const&, bool) [70]
[83]        1.3       1.56       0.05 / 1.50       MahiFit::phase1Apply(HBHEChannelInfo const&, float&, float&, bool&, float&) const
            1.2  .........       1.48 / 1.48         MahiFit::doFit(std::array<float, 3ul>&, int) const [86]
            0.0  .........       0.02 / 0.02         MahiFit::resetWorkspace() const [1301]

            1.2  .........       1.48 / 1.56         MahiFit::phase1Apply(HBHEChannelInfo const&, float&, float&, bool&, float&) const [83]
[86]        1.2       1.48       0.07 / 1.41       MahiFit::doFit(std::array<float, 3ul>&, int) const
            1.0  .........       1.19 / 1.19         MahiFit::minimize() const [121]
            0.1  .........       0.14 / 0.14         MahiFit::updatePulseShape(double, Eigen::Matrix<double, -1, 1, 0, 19, 1>&, Eigen::Matrix<double, -1, 1, 0, 19, 1>&, Eigen::Matrix<double, -1, -1, 0, 19, 19>&\
) const [507]
            0.1  .........       0.07 / 0.07         MahiFit::calculateArrivalTime() const [754]

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25982/8491

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mariadalfonso for master.

It involves the following packages:

CalibCalorimetry/HcalAlgos
CalibCalorimetry/HcalPlugins
RecoLocalCalo/HcalRecAlgos

@perrotta, @tocheng, @cmsbuild, @franzoni, @slava77, @pohsun can you please review it and eventually sign? Thanks.
@mmusich, @tocheng, @argiro this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 20, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33202/console Started: 2019/02/20 10:55

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25982/33202/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 7919 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3098286
  • DQMHistoTests: Total failures: 2153
  • DQMHistoTests: Total nulls: 7
  • DQMHistoTests: Total successes: 3095929
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • DQMHistoSizes: changed ( 136.85 ): -0.012 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 136.731,... ): 0.008 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 4.53 ): -0.004 KiB JetMET/SUSYDQM
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@slava77
Copy link
Contributor

slava77 commented Feb 20, 2019

No changes are expected in the HCAL quantities.

is incompatible with

Reco comparison results: 7919 differences found in the comparisons

At a glance (in the PR), the differences are likely due to changes in double and float precision.
To make sure that there are no actually significant changes, please provide hit-by-hit comparisons for MAHI and M2 for some events with pileup as well as without pileup (to check both the real pulse and noise dominated environments). It would be good to have coverage up to a TeV regime.

@mariadalfonso
Copy link
Contributor Author

@slava77

the difference thisPR/release comes from the mahi arrival time estimation
http://dalfonso.web.cern.ch/dalfonso/HCAL/PR25982/recHitTime_PR25982.png
but actually seems that this PR goes in the direction of bug-fixing it :)
I suspect comes from the initialization of some of the nnlsWork_ matrices, I will understand what exactly is the right line.

@hum

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2019

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25982/33401/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4412 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3114826
  • DQMHistoTests: Total failures: 1271
  • DQMHistoTests: Total nulls: 2
  • DQMHistoTests: Total successes: 3113356
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 31 files compared)
  • DQMHistoSizes: changed ( 136.85 ): 0.004 KiB JetMET/SUSYDQM
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@slava77
Copy link
Contributor

slava77 commented Mar 6, 2019

+1

for #25982 4b3a7c4

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show small differences starting from hbhe rechits and propagating downstream; the differences are consistent with numerical precision level changes
  • local tests with flat-PT dijet 15 to 7TeV with PU50 shows expected behavior, in line with the plots provided in HBHE: mahi speed up #25982 (comment)
    • some example numerology for HE: out of 6,880,364 rechits, there are 1,116,150 with differences in energy. Only 155 have |delta|/sum > 0.0001, of these 0 have |delta| > 0.01 MeV. Most differences are in hits with very small energy values (fits of noise), where fractional changes within numerical precision are more likely.
    • time in hbheprereco is down by about 7%

@VinInn
Copy link
Contributor

VinInn commented Mar 7, 2019

so it was eventually decided that moving float to double (in a fully double algorithm) is bad for physics, wasn't it?

@mariadalfonso
Copy link
Contributor Author

@VinInn I will try to move in the future as much as possible those double to float.

@mariadalfonso
Copy link
Contributor Author

@tocheng can you sign off , the changes are related to the double --> float in the TimeSlew class that is in the CalibCalorimetry package

@slava77
Copy link
Contributor

slava77 commented Mar 7, 2019 via email

@VinInn
Copy link
Contributor

VinInn commented Mar 7, 2019

no vectorization
algos are fully coded in double. Some parameters are in float, so there are conversions float->double that can be avoided.

Anyhow, as @mariadalfonso commented, the best is to migrate everything to float and keep only the critical part of the fit in double...

@slava77
Copy link
Contributor

slava77 commented Mar 7, 2019 via email

@slava77
Copy link
Contributor

slava77 commented Mar 11, 2019

@pohsun @tocheng
ALCA signature is missing for this PR.
Please check and comment or sign.
Thank you.

@pohsun
Copy link

pohsun commented Mar 11, 2019

@mariadalfonso
Are you planning to implement those 'float' in the non-critical parts soon?
Or I could just sign-off?

@mariadalfonso
Copy link
Contributor Author

@pohsun
I consider this PR completed.

@pohsun
Copy link

pohsun commented Mar 11, 2019

+1
@mariadalfonso Thanks for clarification.

@slava77
Copy link
Contributor

slava77 commented Mar 11, 2019

@mariadalfonso Thanks for clarification.
+1

it seems like a "+1" is accepted only if it's in the beginning of the message.
@pohsun please edit your original message or post another "+1"
Thank you.

@cmsbuild
Copy link
Contributor

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)

@fabiocos
Copy link
Contributor

@slava77 @VinInn so do I understand correctly that you suggest/agree to test the version of MAHI proposed by this PR, and possibly consider further updates later on?

@VinInn
Copy link
Contributor

VinInn commented Mar 12, 2019

@fabiocos , yes

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 1313262 into cms-sw:master Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants