Adding the photon kinematics for the EMCal clusters QA type plots #3175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Build & test reportReport for commit 43fe149177589c3fb82a28e2ced30407ef85061b:
Automatically generated by sPHENIX Jenkins continuous integration |
ruse-traveler
suggested changes
Oct 9, 2024
There was a problem hiding this comment.
This is very nice to see! Thank you for the work on this!!
In general, things are in very good shape, but there's a some work to do to get it working within the QA environment. I've written out all of my suggestions in the review comments!
The only other thing is to make sure to add your .h and .cc files to the Makefile!
| private: | ||
| std::string outfilename; | ||
| TFile *outfile; | ||
|
|
There was a problem hiding this comment.
We'll want to add a few members here for bookkeeping:
- 1st is that we'll need to keep the module name on hand for making histogram names: this will help us keep track of what histograms are coming from which modules!
- 2nd is that we might need to change the input node.
- 3rd is that we might need to add a tag to a histogram.
- And then we'll need to know if we're selecting triggers (
doTrgSelect) and, if we are, which one (trgToSelect).
Suggested change
| std::string modulename; | |
| std::string inputnode; | |
| std::string histtag; | |
| uint32_t trgToSelect; | |
| bool doTrgSelect; | |
| int Reset(PHCompositeNode * /*topNode*/) override; | ||
|
|
||
| void Print(const std::string &what = "ALL") const override; | ||
|
|
There was a problem hiding this comment.
Just in case we want to check this as a function of trigger type (e.g. Photon5), we'll need some way to set which trigger to select.
Suggested change
| /// specifies a trigger to select | |
| void SetTrgToSelect(const uitn32_t trig = JetQADefs::GL1::MBDNSPhoton1) | |
| { | |
| doTrgSelect = true; | |
| trgToSelect = trig; | |
| } |
| public: | ||
|
|
||
|
|
||
| photonjetskinematics(const std::string &outputfilename = "output"); |
There was a problem hiding this comment.
While we won't need to specify an output file (since this is managed by the QA histogram manager), we will need to provide the module name since that will be added to the histogram names! And while we probably won't need to change the input node, building in the ability to do so can't hurt...
Suggested change
| photonjetskinematics(const std::string &outputfilename = "output"); | |
| photonjetskinematics(const std::string &modulename = "photonjetskinematics", const std:string &inputnode = "CLUSTERINFO_CEMC"); |
| int Reset(PHCompositeNode * /*topNode*/) override; | ||
|
|
||
| void Print(const std::string &what = "ALL") const override; | ||
|
|
There was a problem hiding this comment.
We'll want to be able to add additional tags to histogram names to label things (e.g. if we're selecting a particular trigger), so we'll need to add a setter.
Suggested change
| /// set histogram tag | |
| void SetHistTag(const std::string& tag) | |
| { | |
| histtag = tag; | |
| } | |
Comment on lines
35
to
40
| photonjetskinematics::photonjetskinematics(const std::string &outputfilename): | ||
| SubsysReco("photonjetskinematics") | ||
| , outfilename(outputfilename) | ||
| { | ||
| std::cout << "photonjetskinematics::photonjetskinematics(const std::string &name, const std::string&outputfilename) Calling ctor" << std::endl; | ||
| } |
There was a problem hiding this comment.
In line with the suggestion in the header; here we can set the module name and input node. We can also hide some of the output messages behind a couple levels of verbosity to keep log files from getting too big!
Suggested change
| photonjetskinematics::photonjetskinematics(const std::string &outputfilename): | |
| SubsysReco("photonjetskinematics") | |
| , outfilename(outputfilename) | |
| { | |
| std::cout << "photonjetskinematics::photonjetskinematics(const std::string &name, const std::string&outputfilename) Calling ctor" << std::endl; | |
| } | |
| photonjetskinematics::photonjetskinematics(const std::string &modulename, const std::string &inputnode): | |
| SubsysReco(modulename) | |
| , modulename(modulename) | |
| , inputnode(inputnode) | |
| , histtag("AllTrig") | |
| , trgToSelect(JetQADefs::GL1::MBDNSJet1) | |
| , doTrgSelect(false) | |
| { | |
| if (Verbosity() > 1) std::cout << "photonjetskinematics::photonjetskinematics(const std::string &name, const std::string&outputfilename) Calling ctor" << std::endl; | |
| } |
Comment on lines
155
to
157
| PHTFileServer::get().cd(outfilename); | ||
|
|
||
| //outfile->cd("photonjetskinematics"); |
There was a problem hiding this comment.
The manager will handle all histogram saving...
Suggested change
| PHTFileServer::get().cd(outfilename); | |
| //outfile->cd("photonjetskinematics"); |
Comment on lines
160
to
164
| h_emcal_cluster_eta_phi->Write("h_emcal_cluster_eta_phi"); | ||
| h_emcal_cluster_energy->Write("h_emcal_cluster_energy"); | ||
| h_emcal_cluster_chi2->Write("h_emcal_cluster_chi2"); | ||
| h_emcal_cluster_eta->Write("h_emcal_cluster_eta"); // Save 1D eta plot | ||
| h_emcal_cluster_phi->Write("h_emcal_cluster_phi"); // Save 1D phi plot |
There was a problem hiding this comment.
Here we instead register the output histograms with the manager rather than save them to a particular file.
Suggested change
| h_emcal_cluster_eta_phi->Write("h_emcal_cluster_eta_phi"); | |
| h_emcal_cluster_energy->Write("h_emcal_cluster_energy"); | |
| h_emcal_cluster_chi2->Write("h_emcal_cluster_chi2"); | |
| h_emcal_cluster_eta->Write("h_emcal_cluster_eta"); // Save 1D eta plot | |
| h_emcal_cluster_phi->Write("h_emcal_cluster_phi"); // Save 1D phi plot | |
| manager->registerHisto(h_emcal_cluster_eta_phi); | |
| manager->registerHisto(h_emcal_cluster_energy); | |
| manager->registerHisto(h_emcal_cluster_chi2); | |
| manager->registerHisto(h_emcal_cluster_eta); // Save 1D eta plot | |
| manager->registerHisto(h_emcal_cluster_phi); // Save 1D phi plot |
| h_emcal_cluster_eta->Write("h_emcal_cluster_eta"); // Save 1D eta plot | ||
| h_emcal_cluster_phi->Write("h_emcal_cluster_phi"); // Save 1D phi plot | ||
|
|
||
| std::cout << "photonjetskinematics::End(PHCompositeNode *topNode) This is the End..." << std::endl; |
There was a problem hiding this comment.
Suggested change
| std::cout << "photonjetskinematics::End(PHCompositeNode *topNode) This is the End..." << std::endl; | |
| if (Verbosity() > 1) std::cout << "photonjetskinematics::End(PHCompositeNode *topNode) This is the End..." << std::endl; |
| //____________________________________________________________________________.. | ||
| int photonjetskinematics::Reset(PHCompositeNode *topNode) | ||
| { | ||
| std::cout << "photonjetskinematics::Reset(PHCompositeNode *topNode) being Reset" << std::endl; |
There was a problem hiding this comment.
Suggested change
| std::cout << "photonjetskinematics::Reset(PHCompositeNode *topNode) being Reset" << std::endl; | |
| if (Verbosity() > 1) std::cout << "photonjetskinematics::Reset(PHCompositeNode *topNode) being Reset" << std::endl; |
| //____________________________________________________________________________.. | ||
| void photonjetskinematics::Print(const std::string &what) const | ||
| { | ||
| std::cout << "photonjetskinematics::Print(const std::string &what) const Printing info for " << what << std::endl; |
There was a problem hiding this comment.
Suggested change
| std::cout << "photonjetskinematics::Print(const std::string &what) const Printing info for " << what << std::endl; | |
| if (Verbosity() > 1) std::cout << "photonjetskinematics::Print(const std::string &what) const Printing info for " << what << std::endl; |
…suggested by Derek.
Build & test reportReport for commit 3646d30039ff4d912afcfb7412b59222afc27d9a:
Automatically generated by sPHENIX Jenkins continuous integration |
ruse-traveler
approved these changes
Oct 16, 2024
There was a problem hiding this comment.
Thanks for the changes!! For some reason I can't mark my previous suggestions as resolved, but I'm happy with this as-is!
The only other comment is a minor one about using m_ prefixes, but I'll leave it up to you whether or not to implement it. (I can always implement it later in a subsequent editing pass.)
| #include <vector> | ||
| //____________________________________________________________________________.. | ||
|
|
||
| photonjetskinematics::photonjetskinematics(const std::string &m_modulename, const std::string &m_inputnode) : |
There was a problem hiding this comment.
Apologies! I should've been more clear: the m_ is for variables that are class members (e.g. the ones starting at line 75 of the .h file), and then the arguments here usually aren't prefixed by anything😅
Merge remote-tracking branch 'upstream/master' into photonQA
Build & test reportReport for commit 36228822f427411b894cc2d48fef1c9b29ceee6b:
Automatically generated by sPHENIX Jenkins continuous integration |
…he build Merge remote-tracking branch 'upstream/master' into photonQA
Build & test reportReport for commit 0dbc266a27b7e64074505a21b5ac52f0cc3ae997:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 4a11793dcfe19ef80d6a217922717b8a5ae5b338:
Automatically generated by sPHENIX Jenkins continuous integration |
ruse-traveler
approved these changes
Nov 18, 2024
There was a problem hiding this comment.
Looks like Jenkins still isn't happy... The only issues are some clang-tidy warnings that aren't related to this PR and cpp-check, which is throwing an error about manager not being initialized in the constructor. Otherwise, all other checks are passed!
I've left a couple of comments in the code about how to fix the cpp-check issue...
| // TFile *outfile; | ||
| // hist manager | ||
|
|
||
| Fun4AllHistoManager* manager; |
There was a problem hiding this comment.
So (hopefully 🤞) this is the last issue: manager not being initialized in the constructor is throwing a warning on Jenkins... I'll add a comment where this needs to be added.
Comment on lines
+48
to
+53
| SubsysReco(m_modulename) | ||
| , modulename(m_modulename) | ||
| , inputnode(m_inputnode) | ||
| , histtag("AllTrig") | ||
| , trgToSelect(JetQADefs::GL1::MBDNSJet1) | ||
| , doTrgSelect(false) |
There was a problem hiding this comment.
Here we'll need to initialize manager, so something like so:
Suggested change
| SubsysReco(m_modulename) | |
| , modulename(m_modulename) | |
| , inputnode(m_inputnode) | |
| , histtag("AllTrig") | |
| , trgToSelect(JetQADefs::GL1::MBDNSJet1) | |
| , doTrgSelect(false) | |
| SubsysReco(m_modulename) | |
| , modulename(m_modulename) | |
| , inputnode(m_inputnode) | |
| , histtag("AllTrig") | |
| , trgToSelect(JetQADefs::GL1::MBDNSJet1) | |
| , doTrgSelect(false) | |
| , manager(nullptr) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Adding the photon kinematics plots (eta, phi, energy and chi squared) for the EMCal clusters.
comment: <> ( What does this PR do? Linking to talk in software meeting encouraged )
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)