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
Updates to pixel phase 1 summary class #18993
Conversation
…nd made updates to summary maps. Grand summary now made in eventInfo folder for automatic GUI purposes, and bins now filled with percentage failing QTests instead of simply warnings/errors.
A new Pull Request was created by @leggat for master. It involves the following packages: DQM/SiPixelPhase1Summary @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
… against not booking plots in the dqmEndLuminosityBlock method
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
|
||
std::vector<std::string> xAxisLabels_ = {"BMO","BMI","BPO ","BPI","HCMO_1","HCMO_2","HCMI_1","HCMI_2","HCPO_1","HCPO_2","HCPI_1","HCPI_2"}; // why not having a global variable !?!?!?! | ||
std::vector<std::string> yAxisLabels_ = {"1","2","3","4"}; // why not having a global variable ?!?!?!!? | ||
std::vector<std::string> yAxisLabels_ = {"1","2","3","4"}; // why not having a global variable ?!?!?!!? - I originally did, but was told not to by David Lange! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first link on google might be informative - google can tell you more about why to avoid using globals in c++ (let alone a multiply threaded c++)
http://www.cplusplus.com/forum/general/76476/
+1 |
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
@@ -69,10 +70,20 @@ | |||
|
|||
std::map<std::string,std::string> summaryPlotName_; | |||
|
|||
//The dead and innefficient roc trend plot | |||
std::map<std::string,MonitorElement*> deadROCTrends_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @leggat - please change these to be std::map<enum,MonitorElement*> to avoid all the string lookups below. (where you should define a set of enums to meet your needs) [best to do this also with the key to summaryPlotName_] Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @davidlange6 - I've changed the deadROCTrends_ and ineffROCTrends_ as you suggest, but I don't immediately see how this would work for summaryPlotName_ since both the key and value are configurable strings passed from the SiPixelPhase1Summary_cfi.py config file. I don't know where I'd start in changing that...
//------------------------------------------------------------------ | ||
void SiPixelPhase1Summary::fillTrendPlots(DQMStore::IBooker & iBooker, DQMStore::IGetter & iGetter, int lumiSec){ | ||
|
||
// If we're running in online mode and the lumi section is not modulo 10, return. Offline running always uses lumiSec=0, so it will pass this test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this comment. why is lumiSec=0 always? (lumiSec seems to be defined as lumiSeg.id().luminosityBlock();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah - the magic is in if (runOnEndJob_)... got it.
Pull request #18993 was updated. @cmsbuild, @dmitrijus, @vanbesien, @davidlange6 can you please check and sign again. |
+1 |
The tests are being triggered in jenkins. |
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
Comparison job queued. |
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
+1 |
Addition of new methods for plotting trend plots:
Dead and inefficient ROC monitoring are the first trend plots to be included.
In addition three changes have made to the summary maps: