Hydjet2 bugfixes #38443

wouf · 2022-06-20T13:40:41Z

PR description:

This is bugfixes for Hydjet2 related to status codes and units issues. The default parameters was updated to Antares_pre tune

PR validation:

workflow 160

SiewYan · 2022-06-20T13:43:59Z

please test workflow 160

cmsbuild · 2022-06-20T13:48:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38443/30642

This PR adds an extra 32KB to repository

cmsbuild · 2022-06-20T13:48:38Z

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

It involves the following packages:

GeneratorInterface/Hydjet2Interface (generators)

@alberto-sanchez, @SiewYan, @mkirsano, @Saptaparna, @GurpreetSinghChahal can you please review it and eventually sign? Thanks.
@alberto-sanchez, @mkirsano this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2022-06-20T20:01:34Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e20d13/25644/summary.html
COMMIT: a3b5590
CMSSW: CMSSW_12_5_X_2022-06-20-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38443/25644/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e20d13/160.0_Hydjet2Q_MinBias_5020GeV_2018_ppReco+Hydjet2Q_MinBias_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECOMB+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3773778
DQMHistoTests: Total failures: 27863
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3745893
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 213 log files, 45 edm output root files, 51 DQM output files
TriggerResults: found differences in 1 / 50 workflows

perrotta

Besides the bug fixes, you are also modifying several generator parameters in hydjet2DefaultParameters_cff.py
Please add it to the PR description, so that it can remain track of it and its purposes

perrotta · 2022-06-21T05:18:45Z

GeneratorInterface/Hydjet2Interface/src/Hydjet2Hadronizer.cc

-  double t = (hj2->GetT()).at(i);
+
+  // convert to mm (as in PYTHIA6)
+  double x = 0.000000000001 * (x0 * cosphi0_ - y0 * sinphi0_);


It is less error prone, and more intuitive, if you define the conversion in a contexpr parameter

How it is possible? We do not know the x0 and y0 at compile time...

e.g.

double constexpr am_to_mm = 0.000000000001; // or even 1e-12, which is more intuitive double x = am_to_mm * (x0 * cosphi0_ - y0 * sinphi0_);

Ah, I see. But what is the reason of constexpr here? Why not just:

const double fm_to_mm = 1e-12; double x = fm_to_mm * (x0 * cosphi0_ - y0 * sinphi0_);

?

Ah, I see. But what is the reason of constexpr here? Why not just:

const double fm_to_mm = 1e-12; double x = fm_to_mm * (x0 * cosphi0_ - y0 * sinphi0_);

?

Well, constexpr is initialized at compile time, while const it is at runtime. Not a big difference here, let it as you updated it.

perrotta · 2022-06-21T05:21:49Z

GeneratorInterface/Hydjet2Interface/test/Hydjet2Analyzer.cc

+            if ((abs(pdg_id) == 130) || (abs(pdg_id) == 310)) {
+              mass = 0.497611;
+            } else {
+              //cout<<"Error! Mass for "<< pdg_id <<" not found in PDT!!!"<<endl;


Does it makes sense to continue if there is a negative mass here?

We using mass only for ET, so with negative mass we still can use other histograms. Anyway this analyser is used for tests and tuning only.

perrotta · 2022-06-21T05:25:24Z

GeneratorInterface/Hydjet2Interface/test/Hydjet2Analyzer.cc

              dheta->Fill(eta);

+              e_sum += e;
+              et_sum += et;
+              if (std::abs(eta) < 1.5) {


if you define (e.g.) etaMaxBarrel , etaMaxAlice , etaMaxCms it is easier to code, and modify afterwards would you ever need it

Well, firstly we have to change detector then)) I thing it is useless to introduce variables like etaMaxBarrel. I prefer to leave it hardcoded in default section, to vary cuts from the configs, it is found userHistos_ section, see this line

cmsbuild · 2022-06-21T19:19:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38443/30663

This PR adds an extra 36KB to repository

cmsbuild · 2022-06-21T19:20:14Z

Pull request #38443 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @alberto-sanchez can you please check and sign again.

SiewYan · 2022-06-27T13:37:18Z

please test workflow 160

perrotta · 2022-07-25T13:36:00Z

@Saptaparna there are quite large changes in wf 160. Since you signed this when the test results were not available yet, could you please confirm that all these changes are as expected, due to the update of the default parameters to Antares_pre tune implemented in this PR?

SiewYan · 2022-07-26T06:48:39Z

Hello @perrotta , echoing from #38443 (comment), I think the large changes are expected as @wouf mentioned the Antares tune is preliminary. I suggest to merge it for now. What do you think @Saptaparna ?

qliphy · 2022-07-28T00:57:59Z

@wouf @Saptaparna Please confirm whether large changes in wf 160 are expected or not. Thanks!

wouf · 2022-07-31T10:15:17Z

@wouf @Saptaparna Please confirm whether large changes in wf 160 are expected or not. Thanks!

What You mean by large changes? No changes in wf within this PR found. Or You mean changes in #PR36316 , in files

Configuration/PyReleaseValidation/python/relval_standard.py
Configuration/PyReleaseValidation/python/relval_steps.py

? If so, i guess, @mandrenguyen can comment used conditions for wf 160.

qliphy · 2022-07-31T10:19:03Z

@wouf You can find from above PR test outcome: "DQMHistoTests: Total failures: 28426"

For more details, please check here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-07-24-2300+e20d13/51778/validateJR.html

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-07-24-2300+e20d13/51778/160.0_Hydjet2Q_MinBias_5020GeV_2018_ppReco+Hydjet2Q_MinBias_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECOMB+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO/

wouf · 2022-07-31T10:28:33Z

@wouf You can find from above PR test outcome: "DQMHistoTests: Total failures: 28426"

For more details, please check here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-07-24-2300+e20d13/51778/validateJR.html

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-07-24-2300+e20d13/51778/160.0_Hydjet2Q_MinBias_5020GeV_2018_ppReco+Hydjet2Q_MinBias_5020GeV_2018_ppReco+DIGIHI2018PPRECO+RECOHI2018PPRECOMB+ALCARECOHI2018PPRECO+HARVESTHI2018PPRECO/

I see, thanks!

@mandrenguyen Could You please to look throw it and to check if wf 160 is correct?

qliphy · 2022-08-10T01:08:28Z

hold
to understand the difference on wf 160

cmsbuild · 2022-08-10T01:08:48Z

Pull request has been put on hold by @qliphy
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

mandrenguyen · 2022-08-10T08:23:29Z

Apologies, I missed that this was waiting for my attention. Wf 160 is exactly the same as wf 158, expect with the new version of Hydjet instead of the old one.
There is no issue with wf 160 per se.

Hydjet is the baseline generator for PbPb events, it's hugely important for the heavy-ion program.
Hydjet++ is a completely rewritten version of this generator, and we're still trying to understand the performance.
IIUC this PR has some genuine bugfixes, as well as a new tuning. Big differences are expected, but Andrey should comment whether these changes go in the expected direction.

Note that even after this PR I don't believe the new generator is necessarily ready for production. There will likely be further tuning necessary, and possibly further bugfixes.

wouf · 2022-08-10T08:28:00Z

Thank You @mandrenguyen . @qliphy , I think we can go ahead, thanks!

qliphy · 2022-08-10T08:44:00Z

unhold
thanks!

cmsbuild · 2022-08-10T08:44:24Z

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. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2022-08-10T08:47:02Z

please test
to refresh the results

cmsbuild · 2022-08-10T12:38:02Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e20d13/26732/summary.html
COMMIT: fbba702
CMSSW: CMSSW_12_5_X_2022-08-09-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38443/26732/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3692476
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3692440
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

qliphy · 2022-08-11T00:59:33Z

+1

Andrey added 2 commits June 20, 2022 15:16

Hydjet2 bugfixes for status codes and units

b310a57

Hydjet2 bugfixes for status codes and units

a3b5590

cmsbuild added this to the CMSSW_12_5_X milestone Jun 20, 2022

cmsbuild added code-checks-pending generators-pending orp-pending pending-signatures tests-pending labels Jun 20, 2022

wouf mentioned this pull request Jun 20, 2022

Hydjet2 status codes bugfix #38092

Closed

cmsbuild added tests-started and removed tests-pending labels Jun 20, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 20, 2022

cmsbuild added tests-approved and removed tests-started labels Jun 20, 2022

perrotta reviewed Jun 21, 2022

View reviewed changes

perrota's comments on units conversation

fbba702

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Jun 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 21, 2022

cmsbuild added tests-approved and removed tests-started labels Jul 25, 2022

cmsbuild added hold pending-signatures and removed fully-signed labels Aug 10, 2022

cmsbuild removed pending-signatures hold labels Aug 10, 2022

cmsbuild added the fully-signed label Aug 10, 2022

cmsbuild added tests-started and removed tests-approved labels Aug 10, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 10, 2022

cmsbuild added orp-approved and removed orp-pending labels Aug 11, 2022

cmsbuild merged commit 1ad0fb5 into cms-sw:master Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hydjet2 bugfixes #38443

Hydjet2 bugfixes #38443

wouf commented Jun 20, 2022 •

edited

SiewYan commented Jun 20, 2022

cmsbuild commented Jun 20, 2022

cmsbuild commented Jun 20, 2022

cmsbuild commented Jun 20, 2022

perrotta left a comment

perrotta Jun 21, 2022

wouf Jun 21, 2022

perrotta Jun 21, 2022

wouf Jun 21, 2022

perrotta Jul 25, 2022

perrotta Jun 21, 2022

wouf Jun 21, 2022

perrotta Jun 21, 2022

wouf Jun 21, 2022

cmsbuild commented Jun 21, 2022

cmsbuild commented Jun 21, 2022

SiewYan commented Jun 27, 2022

perrotta commented Jul 25, 2022

SiewYan commented Jul 26, 2022 •

edited

qliphy commented Jul 28, 2022

wouf commented Jul 31, 2022 •

edited

qliphy commented Jul 31, 2022

wouf commented Jul 31, 2022

qliphy commented Aug 10, 2022

cmsbuild commented Aug 10, 2022

mandrenguyen commented Aug 10, 2022

wouf commented Aug 10, 2022

qliphy commented Aug 10, 2022

cmsbuild commented Aug 10, 2022

qliphy commented Aug 10, 2022

cmsbuild commented Aug 10, 2022

qliphy commented Aug 11, 2022

Hydjet2 bugfixes #38443

Hydjet2 bugfixes #38443

Conversation

wouf commented Jun 20, 2022 • edited

PR description:

PR validation:

SiewYan commented Jun 20, 2022

cmsbuild commented Jun 20, 2022

cmsbuild commented Jun 20, 2022

cmsbuild commented Jun 20, 2022

Comparison Summary

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Jun 21, 2022

cmsbuild commented Jun 21, 2022

SiewYan commented Jun 27, 2022

perrotta commented Jul 25, 2022

SiewYan commented Jul 26, 2022 • edited

qliphy commented Jul 28, 2022

wouf commented Jul 31, 2022 • edited

qliphy commented Jul 31, 2022

wouf commented Jul 31, 2022

qliphy commented Aug 10, 2022

cmsbuild commented Aug 10, 2022

mandrenguyen commented Aug 10, 2022

wouf commented Aug 10, 2022

qliphy commented Aug 10, 2022

cmsbuild commented Aug 10, 2022

qliphy commented Aug 10, 2022

cmsbuild commented Aug 10, 2022

Comparison Summary

qliphy commented Aug 11, 2022

wouf commented Jun 20, 2022 •

edited

SiewYan commented Jul 26, 2022 •

edited

wouf commented Jul 31, 2022 •

edited