Add EBPMT for PMT pairing in EventBuilding #332

fengyvoid · 2025-01-20T03:43:13Z

EBPMT Tool in EventBuildingV2 tool set.

This is splitted from PR #307, I agree that it's definitely easier to submit tools one by one and not necessary to submit by a whole tool chain. I will also list the changes based on your comments in #307.

Changes based on your comments:

· Can we translate EBPMT.cpp line 386 to english please?
Yes, now it's at 396 - 398

marc1uk · 2025-02-24T16:09:19Z

RWMRawWaveforms and BRFRawWaveforms (lines 29,30) are both immediately leaked by lines 46,47.
It would be sensible to either check the return of the corresponding Get calls. It may also be worth checking the corresponding pointers are not null before de-referencing them in the following log lines.

Is it worth moving the check on line 76 to line 44; is this just doing needless work if those checks are both false?

i think line 122 should be enclosed with line 121 in { } for line 120 to prevent spurious printouts.

It seems like it may not be intended that the printout in line 125 may result in the insertion of an element into the AlmostCompletedWavefroms map via operator[].
Same for line 137. Note that .at calls will throw in the event that the element doesn't exist - there may be more logic required to avoid that. I'm getting a bit lost with the different potential if conditions. May be better to check & access the map just once and re-use the result.

Would suggest using references on lines 152, 154, 284, 290, 295, 297, 446, 447, 450-453 to avoid copies.

Not sure if the logic prohibits it but would suggest for good practice the use of emplace rather than operator[] and checking the return value second to avoid potentially clobbering previous entries on lines 325, 326, 327.

1. I move the get and check of `RWMRawWaveforms` and `BRFRawWaveforms` to line 75 to 85. 2. I move the check and return to line 44-49. 3. The line 122 was still at 122, but only print after the `[]` 4. For line 125 and 137, I change the usage of `[]` to a int AlmostCompleteWaveforms_CountHere, initialized to be zero. 5. I would want to use that but may want to leave this to future optimization... since everything is working right now. (not perfectly because I do see zero PMT waveform from some timestamps, but they can be removed in later steps no matter where they are from) 6. Now at line 339 to 341, changed to emplace as you suggest.

fengyvoid · 2025-04-03T21:18:16Z

Appreciate for all the very useful comments and suggestions! I definitely need to learn more about pointers...

I move the get and check of RWMRawWaveforms and BRFRawWaveforms to line 75 to 85.
I move the check and return to line 44-49.
The line 122 was still at 122, but only print after the []
For line 125 and 137, I change the usage of [] to a int AlmostCompleteWaveforms_CountHere, initialized to be zero.
I would want to use that but may want to leave this to future optimization... since everything is working right now. (not perfectly because I do see zero PMT waveform from some timestamps, but they can be removed in later steps no matter where they are from)
Now at line 339 to 341, changed to emplace as you suggest.

Previously, if NumWavesInCompleteSet was set to 140 and there are only ~133 PMT waveforms exist in data, the AlmostCompleteWaveforms[PMTCounterTimeNs] won't be created. But the usage of AlmostCompleteWaveforms[PMTCounterTimeNs] in Log will create an entry there. This won't affect the later code because there are checks to require AlmostCompleteWaveforms[PMTCounterTimeNs] > 5. Now the Log was removed and the tolerance for PMT number is changed to 10 at: ChannelKey.size() >= (NumWavesInCompleteSet - 10)

S81D · 2025-08-25T15:51:41Z

It looks like Marcus' points were addressed in the new changes. As this is a standalone tool and is in the event building workflow currently in use, I say it's good to go.

marc1uk · 2025-09-19T14:07:10Z

UserTools/EBPMT/EBPMT.cpp

+
+
+
+  if (exeNum % 80 == 0 && exeNum != 0)


this is a bit confusing and seems a bit hand wavey. I don't really follow the comment, but am not a great fan of "do the VME offset correction once every X number of executes because that seems to be about right probably".
Is there not a more definitive way of determining when it needs to be done?

marc1uk · 2025-09-19T14:09:50Z

UserTools/EBPMT/EBPMT.cpp

+  vector<uint64_t> RWMEmplacedTimes;
+  vector<uint64_t> BRFEmplacedTimes;
+
+  for (std::pair<uint64_t, std::map<unsigned long, std::vector<Hit>> *> p : *InProgressHits)


would prefer using a reference in range-based for loops, but since value is a pointer i suppose this isn't too critical.

marc1uk · 2025-09-19T14:10:37Z

UserTools/EBPMT/EBPMT.cpp

+  {
+    uint64_t PMTCounterTimeNs = p.first;
+    std::map<unsigned long, std::vector<Hit>> *hitMap = p.second;
+    vector<unsigned long> ChannelKey = InProgressChkey->at(PMTCounterTimeNs);


again making copies of vectors isn't great efficiency, using a reference would be better.

marc1uk · 2025-09-19T14:14:51Z

UserTools/EBPMT/EBPMT.cpp

+    if (ChannelKey.size() >= (NumWavesInCompleteSet - 10))
+    {
+      Log("EBPMT: ChannelKey.size() == (NumWavesInCompleteSet - 1), ChannelKey.size() = " + std::to_string(ChannelKey.size()) + " == NumWavesInCompleteSet - 1 = " + std::to_string(NumWavesInCompleteSet - 1), v_debug, verbosityEBPMT);
+      if (AlmostCompleteWaveforms.find(PMTCounterTimeNs) != AlmostCompleteWaveforms.end())


AlmostCompleteWaveforms is a std::map - no need to use .find() != .end(), just use .count()

marc1uk · 2025-09-19T14:25:22Z

UserTools/EBPMT/EBPMT.cpp

+//      cout<<ch<<", ";
+//    }
+//    cout<<endl;
+auto it_acw = AlmostCompleteWaveforms.find(PMTCounterTimeNs);


again shouldn't be using find with std::map

marc1uk · 2025-09-19T15:20:31Z

UserTools/EBPMT/EBPMT.cpp

+      // loop all the grouped triggers, if the value is target trigger, then calculate the time difference
+      for (int i = 0; i < groupedTriggers.size(); i++)
+      {
+        map<uint64_t, uint32_t> groupedTrigger = groupedTriggers.at(i);


use reference

marc1uk · 2025-09-19T15:22:58Z

UserTools/EBPMT/EBPMT.cpp

+    if (saveRWMWaveforms)
+    {
+      // check does RWMRawWaveforms have key PMTCounterTimeNs, if not, print a log waring and skip
+      if (RWMRawWaveforms->find(PMTCounterTimeNs) == RWMRawWaveforms->end())


std::map.count() instead of .find()

marc1uk · 2025-09-19T15:23:11Z

UserTools/EBPMT/EBPMT.cpp

+    if (saveBRFWaveforms)
+    {
+      // check does BRFRawWaveforms have key PMTCounterTimeNs, if not, print a log waring and skip
+      if (BRFRawWaveforms->find(PMTCounterTimeNs) == BRFRawWaveforms->end())


count instead of find

marc1uk · 2025-09-19T15:23:38Z

UserTools/EBPMT/EBPMT.cpp

+
+  Log("EBPMT: Found " + std::to_string(timestamps.size()) + " timestamps", v_message, verbosityEBPMT);
+
+  // loop timestamps，对于每一个时间戳，检查它与它之前的时间戳的差值是否是8或者16


english translation please

marc1uk · 2025-09-19T15:30:40Z

UserTools/Unity.h

 #include "AssignBunchTimingMC.h"
 #include "FitRWMWaveform.h"
+#include "EBPMT.h"
+#include "EBPMT.h"


duplicate entry

marc1uk · 2025-10-22T00:14:56Z

I'm inclined to merge this to avoid holding things off any longer. The requested changes are very minimal - both with regards to time/effort to implement, but also in terms of severity. In light of this, I'm going to merge, but as per #333 I would appreciate if someone can make (and validate) the requested changes in a new PR.

Add EBPMT for PMT pairing in EventBuilding

b77a41e

fengyvoid added 2 commits April 4, 2025 17:31

The correct file for previous commit

6eb07d8

S81D self-assigned this Aug 22, 2025

S81D added the Ready for Software meeting discussion label Aug 25, 2025

S81D mentioned this pull request Aug 25, 2025

Event builder v2 tools #307

Closed

jminock added Ready for Level 0 Review and merge and removed Ready for Software meeting discussion labels Sep 8, 2025

marc1uk reviewed Sep 19, 2025

View reviewed changes

marc1uk added the question label Sep 19, 2025

marc1uk added waiting for submitter and removed question Ready for Level 0 Review and merge labels Oct 9, 2025

Merge branch 'Application' into EBPMT_PR

7955dbd

marc1uk merged commit 4130676 into ANNIEsoft:Application Oct 22, 2025
1 check passed


		Log("EBPMT: Found " + std::to_string(timestamps.size()) + " timestamps", v_message, verbosityEBPMT);

		// loop timestamps，对于每一个时间戳，检查它与它之前的时间戳的差值是否是8或者16

Add EBPMT for PMT pairing in EventBuilding #332

Add EBPMT for PMT pairing in EventBuilding #332

Uh oh!

Conversation

fengyvoid commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc1uk commented Feb 24, 2025

Uh oh!

fengyvoid commented Apr 3, 2025

Uh oh!

S81D commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc1uk commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fengyvoid commented Jan 20, 2025 •

edited

Loading

marc1uk commented Oct 22, 2025 •

edited

Loading