Trigger decoder for adder triggers #720
Conversation
Digitizer label is already present; LVDS and adder connector information is going to be added to the databases soon.
It can't still be used with `lar::providerFrom` because it's movable.
This is a new feature that allows callers to know if the content provided by the service has changed from the last check or not. It will be used by a coming trigger decoder.
PMT pair bits are assigned to their proper position via a mapping informed by the PMT channel mapping database (which requires an update). In case the information is not available (old database), an option can be activated to allow for a legacy hard-coded fallback. Note that this legacy mapping is known to be incorrect for Run2. The use of the database is enabled by default. A utility to map LVDS bits and PMT channels is also provided (and used by the decoder). Adder bits are not yet mapped in this way.
An updated database is necessary. If an older one is used, a legacy map will be used (which is known not to be completely correct).
It has been found that the FPGS logic assigning a trigger to adder or majority logic may, in case the decision is "both", miss one bit, hence returning a value 3 or 5 instead of the canonical 7. Although this has not been observed in the triggering cryostat information, I am relaxing the decoding logic so that in those cases the trigger is assigned to both cryostats.
That can be majority (PMT pair) trigger, adder trigger or both. This information is stored independently for each cryostat (in case both cryostats are reported as triggering).
PetrilloAtWork
changed the base branch from
develop
to
feature/gp_trigDecoderDB
|
rebased back to develop branch since PR #709 was merged to develop |
jzettle
left a comment
jzettle
left a comment
There was a problem hiding this comment.
I did the best I could. I left a few comments sprinkled about but they are mostly questions for my own understanding of how this will work moving forward.
| /// Encodes the `connectorWord` LVDS bits from the specified `cryostat` | ||
| /// and `connector` into the format required by `sbn::ExtraTriggerInfo`. | ||
| static std::uint64_t encodeLVDSbits | ||
| static std::uint64_t encodeLVDSbitsLegacy |
There was a problem hiding this comment.
What is the idea of "Legacy" here? Is that needed to preserve the ability to decode previous data? How would one know which to use and how is it decided?
There was a problem hiding this comment.
I should clarify that this legacy is about how to interpret the bits from the 64-bit trigger words. "Legacy" is how we interpreted them (or should have interpreted them) before database-based mapping was introduced with this PR. This legacy is not related to the format or version of the input packet, and it does not affect anything commonly used for analysis, including the trigger counts for PoT accounting, or the types of triggers.
It is a fallback for the decoding of those trigger words in case the channel mapping database is not available.
I thought of preserving a bit of the functionality in a possible context where database access is problematic (note however that by default AllowDefaultLVDSmap is false, and the channel mapping service is required).
The "legacy" mapping of the LVDS is "almost right" in the sense that typically the bit is close to where it should be, but it may be not exactly there.
The other case where legacy mapping kicks in is when the database lacks the proper information. Effectively this is a non-issue, since @mvicenzi has updated all run periods with the data needed for the PMT pair mapping, and while the adders information is present only for Run3, but so are the adder bits, so before then there is nothing to decode.
[*] A bit is associated to a defined location according to the prescription in sbn::ExtraTriggerInfo, which says the most upstream PMT end in the most significant bits. This prescription is not fully... prescriptive though: consider for example that the first two bits include PMT channels #0 and #1 respectively, and these two PMT have the same z coordinate.
There was a problem hiding this comment.
The choice of whether the legacy mapping is used or not is automatic -- the only direct control the user has is whether to ever allow it to kick in (AllowDefaultLVDSmap: true) or not (the default).
If allowed, the legacy kicks in if the mapping database is not available (which will be a fatal issue for other decoders, I think). For example, one could think that in the online monitor environment database access is preferably avoided.
Users should not want the legacy mapping for any reason if there is choice.
|
|
||
|
|
||
| std::uint64_t TriggerDecoderV3::encodeLVDSbits | ||
| std::uint64_t TriggerDecoderV3::encodeLVDSbitsLegacy |
There was a problem hiding this comment.
What is the legacy needed for? Is this needed to decode previous data? How do we know which one we need to use and how is it set?
There was a problem hiding this comment.
Yeah I thought I lost the original review comment and didn't realize it preserved it. I only meant to ask that question once...
| std::uint64_t connector01word, std::uint64_t connector23word | ||
| ) const { | ||
|
|
||
| if (!fPMTpairMap) { // use legacy approach |
There was a problem hiding this comment.
What is the functional difference between the legacy approach and the normal one? Does the input string change somehow or are you extracting the information in the same way if you have this map or not?
There was a problem hiding this comment.
Also merged in the answer above.
In short: for the purpose of the trigger 64-bit words, the input string is still and always the same.
| icarus::TriggerConfiguration const* fTriggerConfiguration = nullptr; | ||
|
|
||
| /// Map of LVDS bits. | ||
| std::optional<icarus::trigger::PMTpairBitMap> fPMTpairMap; |
There was a problem hiding this comment.
Is this something pulled from an updated version of one of our databases?
There was a problem hiding this comment.
Yes, that utility is initialised with PMT mapping informartion from standard IICARUSChannelMap service, which together with the database itself was complemented with new information.
You can notice in the code that the constructor of this object requires a reference to the channel mapping service.
| std::string const triggerLogicKey = "MJ_Adder Source " + Side; | ||
| int const triggerLogicCode = data.hasItem(triggerLogicKey) | ||
| ? data.getItem(triggerLogicKey).getNumber<int>(0): 0; | ||
| sbn::bits::triggerLogicMask triggerLogicMask; |
There was a problem hiding this comment.
Just to make sure I understand the additions: There is a new word (or two) in the trigger data packet string than before based on the adders (this one and another)? However, because nothing is being operated on in the online, this does not affect the data fragment creation and just necessitates updating the decoder.
There was a problem hiding this comment.
That is correct. Since there may be two triggers in the two cryostats at the same time, there are two new words, one per cryostat (MJ_Adder Source EAST and MJ_Adder Source WEST). If I remember the fragment creation code correctly, it will split the trigger string in words and read the one it is interested in: we can add freely as long as the required words are still present and not renamed.
There was a problem hiding this comment.
So this is partly true, it does this if we want it to. Otherwise the entire string as it is sent is preserved and sent along with the information in the event. The decoder then presumably has utilities to extract the information from either there directly or the different functions within the fragment.
The new SQLite database is available from `icarus_data` `v09_84_01`. PostgreSQL database is not updated yet.
|
The additional commit is enabling by default the new database, which is now distributed with |
SFBayLaser
left a comment
SFBayLaser
left a comment
There was a problem hiding this comment.
Ok, it looks like all questions, issues are dealt with and so will move to merge for patch release
3 participants
This pull request includes two parts:
These changes need to be supported by a new channel mapping database with the additional information; with the existing database, the code will still work but won't unpack the trigger primitive bits correctly (close to it, but still no).
This PR depends (and effectively includes) PR #709. The release manager can opt to merge this one directly if it is convenient.
Dependency on
icarus_dataneeds to be bumped tov09_84_01, which @SFBayLaser has tagged and deployed on March 20, 2024.Once this pull request is merged into a release, ICARUS trigger decoding will be complete.
Testing
This branch was tested with a data file from run
9435with the new database version, and on a data file from run11790(featuring a mixed majority + adder trigger) with the current database and with the new (not released) one.Reviewers
While this PR is, to my knowledge, non-breaking, it's based on the code breaking #709. Unfortunately that also means that all the code in that PR also appears here, making the review harder.
The new code is expected to correctly process data from the whole ICARUS dataset with no special configuration needs (besides the standard channel mapping service ones).
Note to the release manager
(and to the reviewers if they care)
In order to make the review easier, I have rebased this PR on #709, which this one depends on, instead of its real target
develop.So please once this PR is approved, rebase it back to
developbefore merging (it does make no sense to merge it tofeature/gp_trigDecoderDBbranch, as the PR is configured now).