editing histograms made by ReadTriggerInfo #1319
Conversation
|
☀️ The build tests passed at 359953f.
N.B. These results were obtained from a build of this Pull Request at 359953f after being merged into the base branch at e8fecef. For more information, please check the job page here. |
|
@OBlivate450 please tell us who you are . I cannot figure it out from the information on GitHub. We ask that everyone put enough information on your own GitHub page for your colleagues to know who you are. |
|
I apologize and have since updated my username. Thank you for your patience!
…On Tue, Jul 30, 2024 at 6:57 PM Rob Kutschke ***@***.***> wrote:
@OBlivate450 <https://github.com/OBlivate450> please tell us who you are
. I cannot figure it out from the information on GitHub. We ask that
everyone put enough information on your own GitHub page for your colleagues
to know who you are.
—
Reply to this email directly, view it on GitHub
<#1319 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BJQTTQQUOU2RP456PQ635L3ZPAK5PAVCNFSM6AAAAABLXGHAESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGMZTOMJWHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
📝 The HEAD of |
|
Hi Olivia,
Thanks. It looks like your PR and the Mu2e GitHub membership list picked up your name change. When you get a chance please type the following into the comment box at the bottom on: #1319
@FNALbuild run build test
It tests if the name change is recognized by our continuous integration system ( the system that does a test merge of your PR, does the build and runs a few events from many jobs to make sure that everything is working). If this test passes then I believe that change is complete.
Rob
From: olivia_birney ***@***.***>
Date: Wednesday, July 31, 2024 at 10:22 AM
To: Mu2e/Offline ***@***.***>
Cc: Robert K Kutschke ***@***.***>, Assign ***@***.***>
Subject: Re: [Mu2e/Offline] editing histograms made by ReadTriggerInfo (PR #1319)
[EXTERNAL] – This message is from an external sender
I apologize and have since updated my username. Thank you for your patience!
On Tue, Jul 30, 2024 at 6:57 PM Rob Kutschke ***@***.***> wrote:
@OBlivate450 <https://github.com/OBlivate450><https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OBlivate450-253E&d=DwQFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=9wl1ZBcV1wqwB-2vOubTpA2fj_yx2tEiIEktaxd2KXFvuc4EVrjnqPdXjBL5mnUo&s=tqagtIdY1e1ixwklem5B3tc67HLOsGQx8LLbHenRT5A&e=> please tell us who you are
. I cannot figure it out from the information on GitHub. We ask that
everyone put enough information on your own GitHub page for your colleagues
to know who you are.
—
Reply to this email directly, view it on GitHub
<#1319 (comment)><https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1319-23issuecomment-2D2259337169-253E&d=DwQFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=9wl1ZBcV1wqwB-2vOubTpA2fj_yx2tEiIEktaxd2KXFvuc4EVrjnqPdXjBL5mnUo&s=jRVDV-F_wtVNVORnHxKI0-wYgbD41nZcW2FW0Ijtrtc&e=>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BJQTTQQUOU2RP456PQ635L3ZPAK5PAVCNFSM6AAAAABLXGHAESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGMZTOMJWHE><https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_BJQTTQQUOU2RP456PQ635L3ZPAK5PAVCNFSM6AAAAABLXGHAESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJZGMZTOMJWHE-253E&d=DwQFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=9wl1ZBcV1wqwB-2vOubTpA2fj_yx2tEiIEktaxd2KXFvuc4EVrjnqPdXjBL5mnUo&s=bk1hw5Zlc4MEhT30Ily6pHTUkSo7i1SdyQBgYugQK04&e=>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
—
Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1319-23issuecomment-2D2260780527&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=9wl1ZBcV1wqwB-2vOubTpA2fj_yx2tEiIEktaxd2KXFvuc4EVrjnqPdXjBL5mnUo&s=g3pEE7w2eOTX2hOe8WXjM5qdwz9Eq0_Z2FiKJNlYzT4&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABHY5ZR547VAIK5KLEKPYWTZPD6I7AVCNFSM6AAAAABLXGHAESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQG44DANJSG4&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=Zi5IH-k2_JiK_Wipiv3pwwSXO9UNaHiLP8sisfPz__k&m=9wl1ZBcV1wqwB-2vOubTpA2fj_yx2tEiIEktaxd2KXFvuc4EVrjnqPdXjBL5mnUo&s=bDHIdRArVpVOaWxT3kslibmbqM-Laald2pfRL7Ztdh8&e=>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
|
⌛ The following tests have been triggered for 359953f: build (Build queue is empty) |
|
☀️ The build tests passed at 359953f.
N.B. These results were obtained from a build of this Pull Request at 359953f after being merged into the base branch at b5d3931. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 359953f: build (Build queue is empty) |
|
☀️ The build tests passed at 359953f.
N.B. These results were obtained from a build of this Pull Request at 359953f after being merged into the base branch at b5d3931. For more information, please check the job page here. |
There was a problem hiding this comment.
This is good work.
Most of my comments are about issues that were present before you touched the code but we are using the review process to bring code up to standards. While it's a little unlucky that you get stuck with this, I think that most are straightforward to fix.
Please let me know if you have any questions.
| fhicl::Atom<float> nProcess {Name("nEventsProcessed" ), Comment("Number of events processed" ) }; | ||
| }; | ||
|
|
||
| explicit ReadTriggerInfo(const art::EDAnalyzer::Table<Config>& config); | ||
| virtual ~ReadTriggerInfo() { } |
There was a problem hiding this comment.
A have a few general clean up comments. I know that these were there when you started this work but we are asking everyone who touches a file to upgrade it to our current standards.
Please remove the line for the destructor. The default compiler written default destructor will do the right thing. Our preference is to simply not write a destructor if the compiler written default destructor is OK.
| fhicl::Atom<float> nProcess {Name("nEventsProcessed" ), Comment("Number of events processed" ) }; | ||
| }; | ||
|
|
||
| explicit ReadTriggerInfo(const art::EDAnalyzer::Table<Config>& config); | ||
| virtual ~ReadTriggerInfo() { } | ||
|
|
||
| virtual void beginJob(); |
There was a problem hiding this comment.
This comment is in the same spirit as the first.
Please write this line as
void beginJob() override;
The override is very important. It tells the compiler that this member function overrides a virtual function in the base class. If the signature of the function and the return type do not match a function in the base class, you will get a compile time error. While everything here looks correct, it's important to always use override.
FYI. Declaring a function virtual does NOT do the same as override. The compiler does not know if you intended to override a virtual function in the base class or if you plan to use this class as a base class and the virutal is there because you intend future subclasses to override it.
Once you have used override, the prefix virtual is redundant. So our style is not to include it.
Please do the same for endJob, endSubRun, analyze and beginRun.
| void fillCaloCalibTrigInfo (int Index , const CaloCluster* HCl, caloCalibrationHist_ &Hist); | ||
| void fillOccupancyInfo (int Index , const StrawDigiCollection*SDCol, const CaloDigiCollection*CDCol, occupancyHist_ &Hist); | ||
|
|
||
| void trigValidation_module_cc (int& Index , std::string& PathName, unsigned& IndexLastModule, summaryInfoHist_ &Hist); |
There was a problem hiding this comment.
Please rename this function to remove _module; it's misleading. What is the meaning of _cc here? If it's just c++ then please remove it too. If it's something else please choose a better name.
| _trigHelix. resize(_nMaxTrig); | ||
| _trigEvtPS. resize(_nMaxTrig); | ||
|
|
||
| _trigAll. resize(_nMaxTrig); |
There was a problem hiding this comment.
You can move all of the vector resizes into the intializer list.
_trigAll(_nMaxTrig);
We recommend that if something can be intialized in the intializer list, it should be.
The default constructor of trigInfo_ will be called for each member (also true with resize).
| } | ||
| // for (size_t i=0; i< _trigPaths.size(); ++i){ | ||
| // Hist._hTrigBits->GetXaxis()->SetBinLabel(i+1, _trigPaths[i].c_str()); | ||
| // } |
There was a problem hiding this comment.
With a few exceptions, please no not leave commented out code in files. You can use the git history mechanism to see removed code. One exception is if the code is currently under development but not ready to go. If the code is for debug/testing purposes, we prefer that you keep it present but protect it with a run-time configuration varaible named something like debug_level or verbosity, with 0 meaning no output and higher numbers meaning more output. The case for commenting out such code is if it will consume a lot of resources even if you it is never called.
| @@ -366,6 +325,11 @@ namespace mu2e { | |||
| Hist._h2DTrigInfo[0] = trigInfoDir.make<TH2F>("h2DTrigInfo_map_all" , "Trigger correlation map from all filters" , (_nMaxTrig+2), -0.5, (_nMaxTrig+1.5), (_nMaxTrig+2), -0.5, (_nMaxTrig+1.5)); | |||
| Hist._h2DTrigInfo[1] = trigInfoDir.make<TH2F>("h2DTrigInfo_map" , "Trigger correlation map" , (_nMaxTrig+2), -0.5, (_nMaxTrig+1.5), (_nMaxTrig+2), -0.5, (_nMaxTrig+1.5)); | |||
|
|
|||
| //add a for loop for creating histograms like | |||
| for(int i=0; i<30; ++i){ | |||
There was a problem hiding this comment.
Don't use literal numbers. Is 30 already defined in an another variable? If not please do:
constexpr int variable_with_a_descriptive_name{30};
for ( int i=0; i<variable_with_a_descriptive_name; ++i){
// body of the loop
}
Alternatively, use a simple name but with a descriptive comment.
| @@ -1066,7 +766,15 @@ namespace mu2e { | |||
| const mu2e::TriggerInfo* trigInfo(0); | |||
|
|
|||
| for (unsigned int i=0; i< _trigPaths.size(); ++i){ | |||
| string&path = _trigPaths.at(i); | |||
| // std::string&path = _trigPaths.at(i); | |||
There was a problem hiding this comment.
Please remove commented out code.
| @@ -1066,7 +766,15 @@ namespace mu2e { | |||
| const mu2e::TriggerInfo* trigInfo(0); | |||
There was a problem hiding this comment.
Use nullptr, not 0, to intialize pointers.
| double z_min = std::numeric_limits<double>::max(); | ||
| double z_max = std::numeric_limits<double>::lowest(); | ||
| for (const auto& finter : KSeed->intersections()) { | ||
| if (finter.position3().z() < z_min) { |
There was a problem hiding this comment.
Prefer:
z_min = std::min( z_min, finter.position3().z());
z_max = std::max( z_max, finter.position3().z());
| } | ||
|
|
||
| double lambda = fseg.loopHelix().lam(); | ||
| double pitch = std::abs(lambda)*6.28; |
There was a problem hiding this comment.
Is 6.28 this 2*PI?
constexpr twopi = 2.*M_PI
|
I should also mention that if you push new commits to the branch in your fork, they will automatically be included in the PR. |
|
@gianipez Olivia tells me that she has completed her summer job. When you are back from vacation let's discuss how to move this forward. All of my change requests are for general cleanup that I ask for whenever we touch older files. |
|
📝 The HEAD of |
Eliminated MC histograms, updated code to reflect modern conventions, and created new breakdown histograms to graph events' passage/failure of filters and modules.