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

Pythia Update to fix tau decays #29

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Conversation

Saptaparna
Copy link

The tau patch has been implemented that includes changes to: src/HelicityMatrixElements.cc
and src/TauDecays.cc

The details of the bug are here: https://gitlab.com/Pythia8/releases/-/issues/155

@cmsbuild
Copy link

A new Pull Request was created by @Saptaparna (Saptaparna Bhattacharya) for branch cms/306.

@cmsbuild, @smuzaffar, @aandvalenzuela, @iarspider can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.
cms-bot commands are listed here

@smuzaffar
Copy link
Contributor

please test

@smuzaffar
Copy link
Contributor

please test with cms-sw/cmsdist#8139

@cmsbuild
Copy link

-1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-707b35/28475/summary.html
COMMIT: b603a50
CMSSW: CMSSW_12_6_X_2022-10-24-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-externals/pythia8/29/28475/install.sh to create a dev area with all the needed externals and cmssw changes.

External Build

I found compilation error when building:

Requested to quit.
Requested to quit.
* The action "build-external+pythia8+306-d2214dc7d8e9c337cdb9077b3a27c24a" was not completed successfully because Failed to build pythia8. Log file in /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/BUILD/el8_amd64_gcc10/external/pythia8/306-d2214dc7d8e9c337cdb9077b3a27c24a/log. Final lines of the log file:
|   couplingsPtrIn
src/HelicityMatrixElements.cc: In member function 'virtual void Pythia8::HMETwoFermions2GammaZ2TwoFermions::initConstants()':
src/HelicityMatrixElements.cc:408:11: error: 'couplingsPtr' was not declared in this scope; did you mean 'Couplings'?
408 |   sin2W = couplingsPtr->sin2thetaW();
|           ^~~~~~~~~~~~
|           Couplings
src/HelicityMatrixElements.cc: In member function 'virtual void Pythia8::HMEZ2TwoFermions::initConstants()':
src/HelicityMatrixElements.cc:681:10: error: 'couplingsPtr' was not declared in this scope; did you mean 'Couplings'?


@agrohsje
Copy link

I see in the patched file:
// Copyright (C) 2019 Philip Ilten, Torbjorn Sjostrand.
pointing to 2019. Is the patch really based on P8.307 ?
It would be irrelevant for the above but this seems to affect other places as well, e.g.
limitTau0 = settingsPtr->flag("ParticleDecays:limitTau0");
tau0Max = settingsPtr->parm("ParticleDecays:tau0Max");
limitTau = settingsPtr->flag("ParticleDecays:limitTau");
tauMax = settingsPtr->parm("ParticleDecays:tauMax");
limitRadius = settingsPtr->flag("ParticleDecays:limitRadius");
rMax = settingsPtr->parm("ParticleDecays:rMax");
limitCylinder = settingsPtr->flag("ParticleDecays:limitCylinder");
xyMax = settingsPtr->parm("ParticleDecays:xyMax");
zMax = settingsPtr->parm("ParticleDecays:zMax");
Could it be that this is for an older version or so. I would assume the changes to be significantly less than this commit.

@agrohsje
Copy link

While I wrote the above, the checks "finished". They point to the same direction:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-707b35/28475/cmsswtoolconf.log

@Saptaparna
Copy link
Author

Ok, let me ask the authors then.

@Saptaparna
Copy link
Author

It seems the patch was for 8.307 though.

@agrohsje
Copy link

agrohsje commented Oct 24, 2022

Can you point to the original patch? I don't see why the time stamp should say 2019 if this is 8.307.
I just checked P8.240 and I see this kind of statements:
limitTau0 = settingsPtr->flag("ParticleDecays:limitTau0");
which looks exactly like what you have in the modified version and which seems incompatible with P8.307.

@cmsbuild
Copy link

Pull request #29 was updated.

@Saptaparna
Copy link
Author

please test with cms-sw/cmsdist#8139

@sihyunjeon
Copy link

sihyunjeon commented Oct 25, 2022

Wow this is very puzzling it did go through. I am totally lost now. For example, I am not sure why this has changes for [1] to [2] included in this pull request. [1] is line in 8.2xx and [2] is the corresponding lines in 8.3xx. The default cms-externals was wrong in the first place (not up to date with 8.3xx)? @agrohsje (SORRY FALSE ALARM - I mixed up with the other git commit histories - commented again below)

[1] https://gitlab.com/Pythia8/releases/-/blob/pythia8244/src/TauDecays.cc#L84-92
[2] https://gitlab.com/Pythia8/releases/-/blob/pythia8306/src/TauDecays.cc#L77-85

@Saptaparna
Copy link
Author

I am surprised too -- checking.

@sihyunjeon
Copy link

sihyunjeon commented Oct 25, 2022

No wait I totally misunderstood what you've done @Saptaparna

So this is nothing else other than changing the time stamp (2021->2022). None of the fixes are applied here. So that's why compilation was successful - it's a null pull request that doesn't change anything.

I checked Philips patch file and the changes he made isn't included here

@cmsbuild
Copy link

Pull request #29 was updated.

@Saptaparna
Copy link
Author

please test with cms-sw/cmsdist#8139

@menglu21
Copy link

No wait I totally misunderstood what you've done @Saptaparna

So this is nothing else other than changing the time stamp (2021->2022). None of the fixes are applied here. So that's why compilation was successful - it's a null pull request that doesn't change anything.

I checked Philips patch file and the changes he made isn't included here

since Sapta already included the patch, just put the link of the patch here, https://gitlab.com/Pythia8/releases/-/issues/155#note_1054685915

@agrohsje
Copy link

Thanks Sapta! This looks good to me now. It seems you applied the patch to 8.307 and not the branch. But doesn't matter as the actual code is the same compared to 8.306. Thanks!

@cmsbuild
Copy link

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-707b35/28482/summary.html
COMMIT: 48cc2ee
CMSSW: CMSSW_12_6_X_2022-10-24-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-externals/pythia8/29/28482/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-707b35/28482/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-707b35/28482/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • Reco comparison had 6 failed jobs
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3378384
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3378359
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 201 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@menglu21
Copy link

+1

@smuzaffar
Copy link
Contributor

+externals

@cmsbuild
Copy link

This pull request is fully signed and it will be integrated in one of the next cms/306 IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@Saptaparna
Copy link
Author

Thanks Sapta! This looks good to me now. It seems you applied the patch to 8.307 and not the branch. But doesn't matter as the actual code is the same compared to 8.306. Thanks!

Indeed. I did that to be expedient as that is the setup I had locally. I think next time I will directly apply the patch.

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.

6 participants