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

organize HLT trigger rate plots in directories #17519

Merged
merged 5 commits into from Feb 27, 2017

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented Feb 15, 2017

as requested in JIRA #1158
this update is meant to re-organize HLT trigger paths in sub-directories
in order to speed up the displaying on the GUI

plots are created in directories depending on the PD the trigger belongs to

because the same trigger might be used in different PDs,
extra plots are created w.r.t. previous implementation

2 set of plots are dropped, instead,
they correspond to the paths HLTriggerFirstPath and HLTriggerFirstPath,
which are not needed to be monitored

the new TriggerMonitorRateClient is currently not used

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mtosi (mia tosi) for CMSSW_9_0_X.

It involves the following packages:

DQM/HLTEvF

@perrotta, @cmsbuild, @silviodonato, @dmitrijus, @Martin-Grunewald, @fwyzard, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@mtosi
Copy link
Contributor Author

mtosi commented Feb 15, 2017

it would be useful to have this PR merged in 900pre5,
because --as far as I understood-- 900pre5 will be used in the upcoming WMGR,
and therefore it would be useful to check this new feature on the online DQM

int nbinsX = 0;
int ibinY = 1;
for ( auto const & me : mes ) {
std::cout << "me: " << me << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::cout!

@cmsbuild
Copy link
Contributor

Pull request #17519 was updated. @perrotta, @cmsbuild, @silviodonato, @dmitrijus, @Martin-Grunewald, @fwyzard, @vanbesien, @davidlange6 can you please check and sign again.

@cmsbuild
Copy link
Contributor

Pull request #17519 was updated. @perrotta, @cmsbuild, @silviodonato, @dmitrijus, @Martin-Grunewald, @fwyzard, @vanbesien, @davidlange6 can you please check and sign again.

@dmitrijus
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 15, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17798/console Started: 2017/02/15 14:45

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

// ensure each dataset is incremented only once per event
break;
}
for (unsigned int i = 0; i < m_datasets[d].size(); ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a bug - i is used in two loops at once. Is it really correct?
@mtosi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apparently the comment I wrote 10 days ago was not committed .. it seemed to me so, but it was not !

the comment was
loop opened in line 361 gets closed in 366,
not that the for instance does not have the for (unsigned int i: m_datasets[d])

in addition, the compiler does not complain, above all in this case where the variable i gets different types

Copy link
Contributor

Choose a reason for hiding this comment

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

ah - ok, I missed the lack of a {} for the loop starting on line 361 (the } on line 366 closes the if statement started on 362)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right
is it ok ?
thanks

@mtosi
Copy link
Contributor Author

mtosi commented Feb 20, 2017

ciao
are there pending issue w/ this PR ?
thanks a lot !
@davidlange6 @fwyzard @silviodonato @Martin-Grunewald

@Martin-Grunewald
Copy link
Contributor

@mtosi
Have you seen @davidlange6 comments (see above)?

@mtosi
Copy link
Contributor Author

mtosi commented Feb 20, 2017

yes, last week, but I replied to them, didn't I ?

@Martin-Grunewald
Copy link
Contributor

@mtosi
Look again at the PR, there a new comments on some loop index i.

@davidlange6
Copy link
Contributor

-1
while waiting for comments from @mtosi

@davidlange6
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit f7cdeeb into cms-sw:CMSSW_9_0_X Feb 27, 2017
@mtosi
Copy link
Contributor Author

mtosi commented Feb 27, 2017 via email

@fwyzard
Copy link
Contributor

fwyzard commented Feb 27, 2017

@mtosi

in addition, the compiler does not complain, above all in this case where the variable i gets different types

The compiler does not necessarily complain if you reuse the same variable in a nested loop.
This is valid C++

  for (unsigned int i = 0; i < 3; ++i)
    for (double i = 0.; i < 3.; ++i)
      for (auto const & i: std::vector<std::string>{ "Hello", " ", "world", "\n" })
        std::cout << i;

that will print 9 times Hello world.

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

6 participants