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

fixing missing pointer between lumi boundaries #25850

Merged

Conversation

alberto-sanchez
Copy link
Member

No description provided.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25850/8309

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

A new Pull Request was created by @alberto-sanchez (Alberto Sanchez Hernandez) for master.

It involves the following packages:

GeneratorInterface/Pythia8Interface

@alberto-sanchez, @cmsbuild, @efeyazgan, @perrozzi, @qliphy can you please review it and eventually sign? Thanks.
@agrohsje, @mkirsano 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

@alberto-sanchez
Copy link
Member Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

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

@alberto-sanchez
Copy link
Member Author

Trying to fix #25708

@fabiocos
Copy link
Contributor

fabiocos commented Feb 4, 2019

@alberto-sanchez did you try this in the context of the LS switch? And did you verify the output?

@alberto-sanchez
Copy link
Member Author

alberto-sanchez commented Feb 4, 2019

@fabiocos I run my test at
/afs/cern.ch/work/a/asanchez/public/CMSSW_10_5_X_2019-02-03-0000/src/test2
that includes the test switching between 3 LS, and the job with one single LS.
If I understand correctly both files have the same input, then the hadronization output seems
the same.
I have to admit no more test have been performed.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3097440
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3097242
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@alberto-sanchez
Copy link
Member Author

@fabiocos , local McM validation for PPS-RunIIFall18GS-00007 has been run successfully, with this fix.

@alberto-sanchez
Copy link
Member Author

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2019

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

fabiocos commented Feb 7, 2019

+1

@alberto-sanchez we will need a backport to 10_2_X

if(!infoPtr->eventAttributes) {
fEvAttributes->clear();
infoPtr->eventAttributes = fEvAttributes;
} else {
infoPtr->eventAttributes = fEvAttributes; // make sure still there
infoPtr->eventAttributes->clear();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note that now both branches do the same, so the if-else is redundant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed they are redundant. Should I remove it?.
It is already merged

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alberto-sanchez strictly speaking the code is not ideal and @makortel is right, but the result looks correct and still I think this is a workaround (although I need to look into pythia8 better). In case we want to clean it further you may just have a small PR, and then do the backport.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course if it the real fix might need different code paths for the cases, a temporary cleanup probably doesn't make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually as written the branches are different.. Perhaps the difference is a no-op, but then the code is rather confusing if that is the case.

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

5 participants