New standalone C++ class to create/fill DQM histograms for CRV. - #1949
New standalone C++ class to create/fill DQM histograms for CRV.#1949RobMina wants to merge 5 commits into
Conversation
|
☀️ The build tests passed at 913a6a9.
N.B. These results were obtained from a build of this Pull Request at 913a6a9 after being merged into the base branch at 8fc19ed. For more information, please check the job page here. |
|
📝 The HEAD of |
|
To start, this is an impressive PR, much more than I expected to start the DQMHelper concept. I can't absorb it all, and there are things I don't really understand. Here are few high-level comments
Pending any CRV group comments, if I had to manage this merge, I might just merge and see how it works out, since too much to actually look at or think about. |
…g diagnostic histogram to a single 2D histogram instead of NxN separate hists.
|
Thanks Ray! Sorry that you already started to review, I had a few more updates I wanted to make. I was able to test this using modified versions of the online (https://github.com/Mu2e/otsdaq-mu2e-crv/blob/mu2e/ots_ops/otsdaq-mu2e-crv/ArtModules/CrvDQM_module.cc) and Offline (https://github.com/Mu2e/Offline/blob/main/CRVReco/src/CrvDQMcollector_module.cc) modules. As an example, I ran over one file (raw.mu2e.trk.vst.124155_000001.art) from dataset raw.mu2e.trk.vst.art and the output histogram files are: For comparison, I also ran over a cosmic MC file (dig.mu2e.CosmicCRYExtracted.MDC2025au_best_v1_5.001400_00001974.art) from (dig.mu2e.CosmicCRYExtracted.MDC2025au_best_v1_5.art): |
| crvDaqErrorModuleLabel : "CrvDigi" | ||
| crvDigiDQMDir : "CRVDigiDQM" | ||
| fillInclusiveDigiDQM : true | ||
| # KPP cabling: fold ROC4 onto ROC2. Set false once the full |
There was a problem hiding this comment.
This is a fcl parameter of the DAQ digi generator. The current Pass1 scripts already change ROC 4 to ROC 2, so that all art data should see the correct ROC numbers.
| fhicl::Atom<std::string> crvDaqErrorModuleLabel{Name("crvDaqErrorModuleLabel"), Comment("label of module that found the CRV-DAQ errors")}; | ||
| fhicl::Atom<std::string> crvDigiDQMDir{Name("crvDigiDQMDir"), Comment("TFileService subdirectory for CRVDigiDQM histograms"), "CRVDigiDQM"}; | ||
| fhicl::Atom<bool> fillInclusiveDigiDQM{Name("fillInclusiveDigiDQM"), Comment("also fill BarId/SiPM/ADC in CRVDigiDQM"), true}; | ||
| fhicl::Atom<bool> crvDigiDQMkppReadout{Name("crvDigiDQMkppReadout"), Comment("KPP cabling: fold ROC 4 onto ROC 2 and book h1/h2_channels"), true}; |
There was a problem hiding this comment.
see comment made for fcl file
| std::vector<TH1F*> _histPEsROC; //for each channel | ||
| std::vector<bool> _notConnected; //for each channel | ||
|
|
||
| std::vector<TH1F*> _histDigisPerChannelAndEvent; |
There was a problem hiding this comment.
Is this actually being stored or used anywhere (other than being filled later in the code)? If so, it should be moved to the helper class. If not, it should be removed.
…move _histDigisPerChannelAndEvent into standalone C++ class, and clean up comments.
|
Thanks Ray and Ralf! I merged the two fcls into one so the digi/status histograms live in the same output file. The histogram directory in each is now one level deep instead of two (each file has two separate directories, one for digi and one for status histograms). Regarding the comparison to the dqm.mu2e.cosmics_crv.crvreco file -- there should be some overlap (specifically, the digi histograms), but I have not yet implemented the reco-level histograms in a standalone C++ class. That would be the next step after the implementation details (for example, what lives in DQM and what lives in Offline) are ironed out. I think I addressed Ralf's three comments. |
|
We still need to decide where the modules live - here, in a CRV subdir, or in DQM repo. The metric extractor is in the DQM repo which has to be co-developed. There are several options. The general procedure is to get the stakeholders together: Yuri, Dave, CRV and DQM to hash it out. With the CM next week, we won't have our usual meetings, so that's a big delay. Not sure the best way forward. |

No description provided.