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

LHC Info code review PR #62

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

LHC Info code review PR #62

wants to merge 29 commits into from

Conversation

grzanka
Copy link

@grzanka grzanka commented Jul 19, 2022

PR description:

This PR is not meant to be merged, its solely purpose is the discussion and review the code of new implementation of the LHCInfo classes

@francescobrivio
Copy link

Hi @grzanka please add the new CondFormats to:

@JanChyczynski JanChyczynski force-pushed the jc_lhcInfo_split branch 3 times, most recently from 7688366 to bcc068e Compare August 10, 2022 10:23
@@ -469,8 +504,7 @@ void LHCInfoPerLSPopConSourceHandler::getNewObjects() {
}


size_t nlumi = getLumiData(oms, lhcFill, startSampleTime, endSampleTime);
edm::LogInfo(m_name) << "Found " << nlumi << " lumisections during the fill " << lhcFill;
getLumiData(oms, lhcFill, startSampleTime, endSampleTime);
Copy link

Choose a reason for hiding this comment

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

Can you explain the reason why the printout is no longer executed ?

Choose a reason for hiding this comment

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

It's been moved to line 142, inside getLumiData https://github.com/CTPPS/cmssw/pull/62/files#diff-f26dd73d4300142b911cc877df1bcf2313392b1a0835c7a004d615fdc3f887b8R142
I've decided that getLumiData shouldn't return the number of lumisections it got from OMS but the number of buffered lumisections and the debug info about how many lumisections it has got from OMS should be inside the method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants