SBNSoftware / icaruscode

Main/top level repository for ICARUS specific code
11 stars 33 forks source link

Channel mapping interface change #709

Closed PetrilloAtWork closed 6 months ago

PetrilloAtWork commented 6 months ago

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 IICARUSChannelMapping service, trying to keep as much legacy as reasonably possible.

Breaking changes

IICARUSChannelMap service architecture change

Currently 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 that icaruscode currently provides (ICARUSChannelMappingProvider). This implementation allows the second choice, that is the database backend via a choice of a tool (ChannelMapPostGres and ChannelMapSQLite).

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, and ICARUSChannelMappingProviderSQLite and ICARUSChannelMappingProviderPostGres the 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 Legacy directory 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 IICARUSChannelMap interface 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:

All the code in icaruscode has been updated accordingly, but it was not possible to test it properly.

Configuration changes

Presets for the two backends are provided (icarus_channelmappinggservice_sqlite and icarus_channelmappinggservice_postgres), and the "standard" icarus_channelmappinggservice has been turned into a copy of the SQLite one. In addition, the configuration icarus_channelmappinggservice_legacy should 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_channelmappinggservice will 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 debug mode, where everything is slow anyway), but if that turns out to be a problem a ~hack~ solution 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 from 0x1008 to 0x1110 are all reported to be on flange EE02 instead of EE03 to EE19. 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 renamed ChannelMapDumper.

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:

In 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).

mmrosenberg commented 6 months ago

trigger build SBNSoftware/sbnobj#106

FNALbuild commented 6 months ago

:heavy_check_mark: CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

FNALbuild commented 6 months ago

:heavy_check_mark: CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

FNALbuild commented 6 months ago

:x: CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

:rotating_light: For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild commented 6 months ago

:warning: CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

:rotating_light: For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

PetrilloAtWork commented 6 months ago

I confirm that this PR also comes with my commitment to contribute to maintenance of aforementioned code.

SFBayLaser commented 6 months ago

Merging this to set up a patch release of icaruscode