Channel mapping interface change #709
Conversation
…ng dump Except for CRT calibration information.
The code of SQLite tool to access the database has been untoolised and it is now used directly by a new service `ICARUSChannelMapSQLite`. The service provider `ICARUSChannelMapSQLiteProvider` is completely art-independent.
Now instead of chooising a service (one option) and a tool (two options) we choose just the service (two options, plus a legacy one).
Because it dumps almost all channel maps.
Instead of hard-coded column numbers.
The public interface is unchanged and still confusing as always.
… numbers Except the CRT calibration database.
The interface for PMT channel mapping is changed:
* the main call to receive PMT mapping information was renamed
from `getChannelIDPairVec()` to `getPMTchannelInfo()`
* the name of the returned type was renamed from `DigitizerChannelChannelIDPairVec`
to `PMTdigitizerInfoVec`
* more importantly, the data for each channel, which was stored in a triple,
`std::tuple<std::size_t, std::size_t, std::size_t>` with alias name
`DigitizerChannelChannelIDPair`, is now a different object, a record (`struct`)
named `PMTChannelInfo_t`. The update instructions are that where a value `t`
was obtained as an element of the vector from `getChannelIDPairVec()`, now
`t` is obtained as element of the return of `getPMTchannelInfo()` and access
needs to be changed as follows:
1. `std::get<0>(t)` becomes `t.digitizerChannelNo`
2. `std::get<1>(t)` becomes `t.channelID`
3. `std::get<2>(t)` becomes `t.laserChannelNo`
All code in `icaruscode` has been updated accordingly.
PetrilloAtWork
added
enhancement
Because otherwise the CET build system does not install it and standalone ROOT/cppyy can't load the class.
Also this fixes another bug that I had missed, again from copy&paste. Some information that is not currently in the database is still hard-coded.
|
trigger build SBNSoftware/sbnobj#106 |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
SFBayLaser
left a comment
SFBayLaser
left a comment
There was a problem hiding this comment.
Blessed are the changes for they are good and seen to provide for the benefit of the detector for many years to come.
And blessed is Gianluca for maintaining this code for many years to come.
This PR therefore dutifully blessed as per Gianluca's request!
|
I confirm that this PR also comes with my commitment to contribute to maintenance of aforementioned code. |
|
Merging this to set up a patch release of icaruscode |
5 participants
This is a large pull request that is actually the preparation for a decoder able to extract all the information from the trigger hardware. It depends on SBNSoftware/sbnobj#106 and will be followed by a final PR.
The new decoder is going to use additional information from the channel mapping database, which begs for a change in interface. I took the chance to sort a bit the architecture of the
IICARUSChannelMappingservice, trying to keep as much legacy as reasonably possible.Breaking changes
IICARUSChannelMapservice architecture changeCurrently channel mapping includes two backends: to a file-stored SQLite database and to a networked PostGreSQL database.
The setup of the channel mapping is done by in two steps.
The first is the "choice" of the implementation for the service,
IICARUSChannelMap, among the one implementations thaticaruscodecurrently provides (ICARUSChannelMappingProvider).This implementation allows the second choice, that is the database backend via a choice of a tool (
ChannelMapPostGresandChannelMapSQLite).The rearchitecturing in this pull request quenches the two steps into choosing among two implementations of the service
IICARUSChannelMap, one bound to the SQLite backend (ICARUSChannelMappingSQLite), the other to the PostGreSQL one (ICARUSChannelMappingPostGres).In addition, the services now follow the service/provider protocol, that is the service is a lightweight object wrapping an art-independent service provider class (interface
IICARUSChannelMappingProvider, andICARUSChannelMappingProviderSQLiteandICARUSChannelMappingProviderPostGresthe implementations).While the old service also follows more or less this protocol, its provider (
ICARUSChannelMappingProvider) is not art-independent due to the use of art tools.The new art-independent service providers should be loadable in C++, ROOT and Python scripts. For the latter, a SBNSoftware/icarusalg pull request will introduce an extension to load them from the interface configuration (
service.IICARUSChannelMap), which right now is not supported.Under the hood most of the code is still the same, so much so that the previous system, the single service and the two tools, are still present, confined in the
Legacydirectory but otherwise still operational, and they share the backend code with the new services.The hope is that one day we'll grow tired of maintaining the legacy code and will drop it.
PMT channel mapping information and interface
The
IICARUSChannelMapinterface is notorious for having confusing names.A coming pull request is going to extend the PMT information that can be retrieved from the databases, so that breaking change seems a good chance to also rationalise the naming at least for the part of the interface that would break anyway.
The changes:
getChannelIDPairVec()togetPMTchannelInfo()DigitizerChannelChannelIDPairVectoPMTdigitizerInfoVecstd::tuple<std::size_t, std::size_t, std::size_t>with alias nameDigitizerChannelChannelIDPair, is now a different object, a record (struct) namedPMTChannelInfo_t. The update instructions are that where a valuetwas obtained as an element of the vector fromgetChannelIDPairVec(), nowtis obtained as element of the return ofgetPMTchannelInfo()and access needs to be changed as follows:std::get<0>(t)becomest.digitizerChannelNostd::get<1>(t)becomest.channelIDstd::get<2>(t)becomest.laserChannelNoAll the code in
icaruscodehas been updated accordingly, but it was not possible to test it properly.Configuration changes
Presets for the two backends are provided (
icarus_channelmappinggservice_sqliteandicarus_channelmappinggservice_postgres), and the "standard"icarus_channelmappinggservicehas been turned into a copy of the SQLite one. In addition, the configurationicarus_channelmappinggservice_legacyshould use the legacy service with SQLite backend, like it was by default, while there is no preset for the legacy service with PostGreSQL backend.As such, configurations that used the default
icarus_channelmappinggservicewill implicitly start using the new service with SQLite backend.Other changes
Besides the following changes, I have fixed, completed, rewritten or added lot of documentation to the code.
Database queries
The queries in both backends were coded so that a fixed column number would be assumed to contain a certain variable (for example, a channel ID expected on column
0).That is quite fragile to start with, it's a maintenance nightmare and there have been instances of the database column order changing with a database update.
The new code in both backends discovers the column number based on the column name, and that is the hard-coded information. It turns out that our queries are dumb ones which load the whole table at once, so the information about which column is which was already there waiting to be interpreted and used.
PostGreSQL backend should be smooth with this change, discovering the columns once per table.
SQLite callbacks are less smooth, and the columns are evaluated once per row, which may be daunting when querying the TPC database. I haven't noticed slowdowns (but I was developing in
debugmode, where everything is slow anyway), but if that turns out to be a problem ahacksolution can be deployed.TPC mapping bug
The function
BuildTPCFragmentIDToReadoutIDMap()maps a fragment ID to its flange code (e.g.EW02) and readout board numbers. The flange code is actually hard-coded instead of coming from the database, and the hard-coding presents a copy&paste error so that fragments from0x1008to0x1110are all reported to be on flangeEE02instead ofEE03toEE19. The readout board numbers appear to be correct.Both SQLite and PostGreSQL backends are affected by the bug.
This pull request has fixed the hard-coded mapping, although I have the intention to replace it to the actual result from the database query if possible.
I am assuming that this bug was not affecting anything, since it's big enough tat it would have been noticed.
Database dumper
The "utility"
PMTChannelMapDumper, which was dumping on screen the whole information we extract from the PMT channel mapping database, has been extended to dump also all the TPC information (that's a lot) and the CRT mapping information, with the exclusion of the calibration (because the interface does not allow to discover the keys to make queries about). This was instrumental in checking that all the code changes did not affect the results of the queries. Given the new scope of the utility, it has been renamedChannelMapDumper.For the reviewers
The database access code has been verified to yield the same results as the old one did, using the utility
ChannelMapDumper. The queries to the CRT calibration database were not included in this test.Stage0 decoding has been successfully run with a Run2 data file.
Some additional code changes to update the interface were not explicitly tested, and I am asking the reviewers specifically to look at them to confirm they make sense:
icaruscode/Decode/DaqDecoderICARUSPMT_module.cc: I actually tested this one a bit better, but @mvicenzi is welcome to cross-checkicaruscode/Decode/DecoderTools/PMTconfigurationExtractor.cxx: this is also mainly my responsibility, and again @mvicenzi is welcome to cross-checkicaruscode/PMT/Calibration/PMTLaserCalibration_module.cc: please @mvicenzi or the unknown-to-SBNSoftware S.C. check this oneicaruscode/Timing/PMTWaveformTimeCorrectionExtractor.cxx: this is for @mvicenziIn addition, I would like @SFBayLaser to bless the technical change.
This pull request blocks one in SBNSoftware/icarusalg to load the channel mapping provider in Python scripts and, more importantly, a trigger decoder update that should go in before keep-up for Run3 starts.
Both these requests are at this time not published (the latter is not even nearly ready, but it will come, and soon too).