Skip to content

Feature/aheggest crt t1#459

Closed
aheggest wants to merge 39 commits intodevelopfrom
feature/aheggest_crtT1
Closed

Feature/aheggest crt t1#459
aheggest wants to merge 39 commits intodevelopfrom
feature/aheggest_crtT1

Conversation

@aheggest
Copy link
Copy Markdown
Contributor

Use CRT Hit time now saved in T1 (T0 wrt GT) as the CRT PMT Matching module, tiny fix in top CRT Hit reco loop

ascarpel and others added 30 commits May 13, 2022 09:32
* the update with a new run was already removed from the interface
* map access replaced with read-only one
* default values for missing entries explicitly implemented
* algorithms use `icarusDB::PMTTimingCorrections` interface (can store it in a constant reference data member)
* art modules provide them the object with that interface via the `icarusDB::IPMTTimingCorrectionService` service interface:
  `lar::providerFrom<icarusDB::IPMTTimingCorrectionService>()` or equivalent returns the pointer the algorithms need
* configuration must include the service `IPMTTimingCorrectionService` and specify its implementation
  (currently only `PMTTimingCorrectionService` is provided
This is effectively a configuration check.
* added documentation
* added default values and magic constants
* added a way to determine if the correction should be considered valid
PetrilloAtWork and others added 9 commits September 12, 2022 21:41
Including:

* including only needed headers
* data members made constant when possible
* configuration checked as early as possible
* unfolded large if statements
* used range-for loops when iteration index not needed
* used "modern" art facility to read data product
* rely on art exceptions for missing data products

Also, fixed the copy of FastToTotal(), which was missing.
Previously, the corrections were in ophitcorr, which was then used for creating flashes.
@rennney
Copy link
Copy Markdown
Contributor

rennney commented Sep 30, 2022

I recommend looking at PR #458. It will bring changes to fcl structure for stage0 stage. It seems like it is very relevant to this PR and will most likely break a thing or two (@aheggest @mmrosenberg )

@wesketchum
Copy link
Copy Markdown
Contributor

wesketchum commented Sep 30, 2022

@SFBayLaser is this critical for the upcoming 2022B release? We're marking 'high' priority right now, but if we should make sure this gets in, update priority please.

@rennney
Copy link
Copy Markdown
Contributor

rennney commented Sep 30, 2022

Hi @wesketchum , I will add a note. This should be important as this PR with combination with #440 should be a first step in reducing cosmic background and potentially reducing the amount of data that we would need to read from tape for reprocessing

PS. My concern with this particular PR is that it has all of commits from #440 in it... which is bad, because #440 is not finished yet. So I would advise @aheggest to remake it only with proper changes

@aheggest
Copy link
Copy Markdown
Contributor Author

aheggest commented Oct 5, 2022

hello @rennney, I will make a new PR with only the relavent changes included. Thanks!

@aheggest aheggest closed this Oct 5, 2022
@aheggest aheggest deleted the feature/aheggest_crtT1 branch June 17, 2023 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants