Skip to content
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

Make optical flash information filling detector-agnostic #282

Open
PetrilloAtWork opened this issue Aug 30, 2022 · 6 comments
Open

Make optical flash information filling detector-agnostic #282

PetrilloAtWork opened this issue Aug 30, 2022 · 6 comments
Labels
bug Something isn't working

Comments

@PetrilloAtWork
Copy link
Member

The code filling the optical flashes was written with ICARUS in mind (see also the review in PR #273).
While this is good for short term, the code needs to be updated.
Incriminated code:

int countingOffset = 0;
if ( cryo == 1 ) countingOffset += 180;
for ( int PMT = 0 ; PMT < 180 ; PMT++ ) {
if ( PMT <= 89 ) sumEast += flash.PEs().at(PMT + countingOffset);
else sumWest += flash.PEs().at(PMT + countingOffset);
}

In addition there is somewhere ICARUS-specific code that relies on an unofficial convention (assigning cryostat number according to the data product label); that should also be fixed.

Relevant people:

  • @JackSmedley: original code
  • @PetrilloAtWork: can propose geometry-based algorithms
  • anybody from SBND (@ikatza?): make sure we don't mess up with east/west wall and other concepts
@PetrilloAtWork PetrilloAtWork added the bug Something isn't working label Aug 30, 2022
@PetrilloAtWork
Copy link
Member Author

Labelling "bug" because as it is, this is not compatible with SBND.

@JackSmedley
Copy link
Contributor

Thanks for resurfacing this, I really did not like leaving it this way. For ICARUS the idea of counting the PEs in one flash on each side of the cathode seemed obviously useful, but since SBND is optically isolated on either side of the cathode I am not sure this kind of accounting makes any sense. Maybe totaling PEs on coated and uncoated PMTs instead of east and west might be useful? Admittedly, I do not know much about SBND.

As a tangential note, I needed to do this funny offset thing in line 129 because the flashes in the east cryostat and west cryostat have different numbers of entries. The east cryostat flashes have a sensible 180 entries, with the first 90 being east wall and the other 90 being west wall. The west cryo has 360 elements, the first 180 are all entirely empty and the second set of 180 follow the same pattern as above. It feels silly to carry around these leading zeros, and without them this could look a lot prettier.

@ikatza
Copy link
Member

ikatza commented Sep 5, 2022

Unless it has changed recently SBND doesn't refer to East and West walls, but it might be useful that it starts to. The single cryostat of SBND is equivalent to the ones from ICARUS, with a wall of optical detectors on the sides.

I believe it is useful to have the sum of PEs on each wall, whilst most interactions will only have PEs on one side, it's possible that particles cross the cathode and a flash has PEs on both walls. I actually use the sum of squared PEs on my flash matcher to decide where a flash centre should be, but I don't store the sum on the SR. I loop through the OpHits of a flash and use the Geometry Service to get their coordinates to decide which wall to sum in this function:

double FlashPredict::wallXWithMaxPE(const OpHitIt opH_beg,

Bare in mind that the function above returns the X position of the wall that has more PE2, the map opdetX_PE has the sums.

@wesketchum
Copy link
Contributor

@ikatza , did your PRs that recently went in address this? Or did those not touch filling into the CAFs?

@ikatza
Copy link
Member

ikatza commented Dec 19, 2022

I did not address this. I did add some new variables to the CAFs, but unrelated to this.

@JackSmedley
Copy link
Contributor

JackSmedley commented Dec 19, 2022

I'd like to refer back to my original comment here. The slightly tricky part is the fact thatrecob::OpFlash.PEs has varying numbers of entries. ICARUS east carries 180 entries (one per PMT in the east cryostat), ICARUS west has 360 (one per PMT in the whole detector despite the fact that the first 180 are always 0), and I don't recall the exact number for SBND (N_PMTs + N_ARAPUCAs?).

  • If an SBND expert could verify for me which entries in this vector correspond to east and west, we could check the length of the vector to determine the correct detector and sum accordingly.
  • If recob::OpFlash.PEs was standardized to have a length equal to the number of relevant photodetectors where the first half corresponds to east and the second half to west or vice versa, then we could just sum the separate halves without ever caring about SBND vs. ICARUS E vs. ICARUS W.

If there is some more clever solution I am missing here, please let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants