Report FATAL error when ext_data_clnt::setup() failed to map all items. #712
Conversation
|
In theory, we might want to have a way to whitelist items which are considered optional. In practice, all of the unpackers which do not use califa.spec seem to be affected by #705, so the need to support unpackers which do not use the per-hit califa wrts field is not there for now. |
|
@e12exp Do I understand correctly that (at least for CALIFA), all unpackers that would exit() with this patch would otherwise deliver bad data, such that the exit() ensures data quality by forcing update of the unpackers? |
|
With the FATAL option we will have problems with the data processing of many experiments: Example for the s515 experiment: NeuLAND: MWPCs: Example for the s455 experiment (part 1): [WARN] R3BUcesbSource.cxx:173:InitUnpackers(): ext_data_clnt::setup() failed TPATFAIL 692 (0x04) not found We have general data structures for the SOFIA detectors that contain: Summary per experiment:
|
Philipp aka @klenze aka @e12exp here. CALIFA_OV is only defined for s509 and s522. All the others should be considered broken. The case where just the WRTS per channel are missing does not appear. |
|
I believe some list of patterns (e.g. "SOFSCI4.*") of unpacker items that are expected to be missing needs to be filled from either the code which sets up the structures, or some higher-level using code (whichever is most convenient), for the various experiments. Of course, if such items are found, it is then also an error, as expectations need to be corrected. |
|
I would like to move the conversation occurring in #705 here. In the analysis WG meeting, this issue was discussed. Although we recognise the importance of making unpacker and R3BRoot consistent, practically it is not easy as the detector setup evolves time-to-time. If we would like to keep compatibility for the old experiment, it is often that we don't have the same number of channels that we have right now. Of course, it is highly encouraged to sort these issues as much as possible, can it be WARNING instead of FATAL, or set a variable accessible from outside of the class to ignore the mismatch? |
|
@ryotani the best option could be to create data structures with mandatory and optional variables. In the case of SOFSCI one scintiallator is always mandatory, the others can be optional. For mwpcs, mwpc0 is mandatory, the others are optional. Maybe upexps experts have more ideas today ... |
|
It should be is possible to handle changing number of channels between experiments with one code by declaring the data structures either with varying size (likely not nice) or by letting them have the largest size needed (for any experiment). Then only request filling of the size (number of array members) needed by the experiment, i.e. the same size as the relevant unpacker provides. To do this, one should not just copy generate The For further code reduction, when several blocks of same variables are needed, the amount of code code could be reduced by putting some loop around it and then having the names generated in parts. |
inkdot7
force-pushed
the
struct_map_failure_fatal
branch
from
2b09b0b
to
c3b62ab
Compare
|
Just a heads up: fairroot has merged their bugfix (FairRootGroup/FairRoot#1307) two weeks ago. This PR will then not be needed, but it would probably be good to have in the meantime, such that r3broot is kept clean of these kinds of unpacking errors. A way to fix the mappings was suggested in my last comment above dated Dec 15. |
inkdot7
force-pushed
the
struct_map_failure_fatal
branch
from
c3b62ab
to
1d8fc16
Compare
inkdot7
force-pushed
the
struct_map_failure_fatal
branch
from
1d8fc16
to
bc12fa1
Compare
I guess @YanzhaoW will comment on it soon |
|
@inkdot7 @jose-luis-rs We are currently using FairRoot 18.8 patch both in CI and lxir136. In the CI, it's directly sourced from cvmfs in a gsi server. I will look into it if we could get the update from that patch. |
inkdot7
force-pushed
the
struct_map_failure_fatal
branch
2 times, most recently
from
d22b2cd
to
e16e9d5
Compare
inkdot7
force-pushed
the
struct_map_failure_fatal
branch
from
e16e9d5
to
3457748
Compare
|
Merging this PR will make users aware of very severe unpacking errors and it has now been around for 6 months. It passes CI. Could we now merge this, such that users become aware of related unpacking issues and thus are urgently encouraged to solve them. This is a direct request from the UCESB author and maintainer. UCESB does not want to be associated with erroneous unpacking due to lacking checks. |
In #862 I have included a new repo to check the unpacking, but we need access to upexps to ensure that everything works. |
|
upexps is here (https://git.gsi.de/r3b/upexps), but not public. I do not know if it has been scrutinized to be made publicly available. Its CI does not pass (does not even start), so general status unknown. @bl0x , @hanstt may have more info. |
|
One thing we should think about is how to handle less-than-maximum-length conditions. For CALIFA, we store the data in one-dimensional structures, which fail gracefully if the array length provided by the unpacker is shorter. For detectors which use onionized multi-dimensional arrays which get mapped to multiple 1d fields, things are more precarious? The fact that complaints about SOFTOFW_P13E2 appeared likely means that if there was some experiment where just P1 to P12 were in use, this would actually blow up? What is a good approach here? Make every slice except for the first optional? Have the caller state how many PSPs (or TOF or Neuland planes) they expect to be present, and assert we have exactly as many? \begin{musing} In the long run, I guess there could be a better system to handle what is presently done by the auto-generated EXT_STR_h101_FOO_ITEMS_INFO. I am still unsure what the best way would be, though. Reflection is not really a strong point of C++. ~~ In theory, you could have a macro which defines a single entry struct with a constructor handling the setup, and then multi-inheriting from all the classes defined by macro calls ~~ actually you can not, because the spoilsports at ISO do not allow to either define a nameless class nor use a class definition instead of a class name in either a template parameter (pack) or an inheritance specification. So, due to inherent limitations of the template mechanism, we can't do this. The only part of the language which can stringify arguments is the preprocessor. If we really want to avoid spelling out all the variables twice we could write a higher order macro. Loops in the preprocessor are possible but somewhat scary. We would be better of autogenerating the setup code (with loops over onion structs and everything) in python. \end{musing} Also, I think this PR should be merged. (Possibly after CI is including recent unpackers, if that can be merged quickly.) |
Indeed, that would blow up in the post-setup check of When only asking mapping of fewer items than are available in the data structures, R3BRoot must then not look at the contents of the further (non-requested) data items, or it might copy non-filled data! That is a problem outside the scope of the external data interface (since it does not know about those members), but one which needs to be carefully thought about!
It sounds reasonable to require the first slice, since if that does not exist, one should not have asked for that data structure at all. I would also suggest to loopify (manually!) the calls to Carthago delenda est: Please consider merging this PR now! - avoid bad unpacking by mistake should be higher priority than disruptions. |
|
In ucesb/ext_data interface, I.e. Carthago delenda est: Please consider merging this PR now! - avoid bad unpacking by mistake should be higher priority than disruptions. |
|
This PR requires the dev branch of FairRoot, is everything ready at GSI to manage the unpacking? |
As far as I know, this patch does not require the dev branch of FairRoot. (As shown by passing CI?) It rather alerts to mapping errors without having the newer FairRoot. However: newer FairRoot versions will by themselves report fatal, since they now care about the false return from |
|
Ok, but at the moment we don't have CIs to check the unpacking process! |
At the moment, I get reports about bad data being mapped due to this missing check. Yes - this will break things for people with broken mappings. But that must surely be better than to continue analyse with unknown data? |
|
And... any affected users surely can do a local modification that changes the |
|
Yes, so I will merge it now. |
|
Hi, I'm working on the unpacker, 202002_s467 to eliminate the issues for
undefined mappings. Although most of them could be solved, I could not
solve some values for CALIFA.
```
CALIFA_TRGENE 48788 (0x04) not found
CALIFA_TRGENEI 48792 (0x04) not found
CALIFA_TRGENEv 48800 (0x04) not found
```
Apparently, they have been defined in the PR #591 by @jose-luis-rs.
However, I cannot find any description of these variables in unpacker in
the gsi server. I think this will affect everyone who's using
CalifaFebexReder. If you could instruct us on how to define them, that
would be great. @jose-luis-rs @klenze, do you know anything about this?
@jose-luis-rs I have another question concerning in the commit you made in
SOFIA repo:
R3BRootGroup/sofia@c6d55c4#diff-9516b3f0736fb252dbb639a396cbbc71d7134738dcc5a0cb3efd9d48ef3f1adfL55-R99
You changed the name of TIMESTAMP of SOFIA from TIMESTAMP_SOFIA_WR_T1 to
TIMESTAMP_SOFIA1WR_T1. As far as I can see in the gsi server, the unpackers
using SOFIA detectors, such as s455, define it as TIMESTAMP_SOFIA_1_WR_T1.
Do you think it's problematic to change the R3B side to adapt to the other
unpackers?
…On Mon, 8 May 2023 at 09:53, Jose Luis Rodriguez ***@***.***> wrote:
Merged #712 <#712> into dev.
—
Reply to this email directly, view it on GitHub
<#712 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD6ZAF3GMFAZZ7GFDIINV53XFCX7FANCNFSM6AAAAAASUZXNLM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
As far as I know, this is the length of the T1 trigger from main DAQ which we use for validation, as measured by FEBEX (as an amplitude). The CALIFA DAQ is monitored using go4, where we bypass ucesb, so I did not put these "special channels" into any mapping. To my knowledge, @jose-luis-rs added the mapping. I assume he did so because simply adding them as channels (perhaps crystal ids 0xffff and 0xfffe) would cause spikes in the histograms of all mapped channels. However, "all mapped channels" is not a terribly useful thing to plot anyhow, for energies, you would mix both gain ranges, for timing differences, you would mix both sides. My recommendation would be to encode the special channels with special crystal ids, and register that crystal id as being special. |
These special channels were implemented in 2021 after the fission experiments(s455), so in your case you cannot define them. Sorry for that. @jose-luis-rs I have another question concerning in the commit you made in SOFIA repo: R3BRootGroup/sofia@c6d55c4#diff-9516b3f0736fb252dbb639a396cbbc71d7134738dcc5a0cb3efd9d48ef3f1adfL55-R99 You changed the name of TIMESTAMP of SOFIA from TIMESTAMP_SOFIA_WR_T1 to TIMESTAMP_SOFIA1WR_T1. As far as I can see in the gsi server, the unpackers using SOFIA detectors, such as s455, define it as TIMESTAMP_SOFIA_1_WR_T1. Do you think it's problematic to change the R3B side to adapt to the other unpackers? What do you mean with "R3B side to adapt to the other unpackers"?
|
|
Hi @klenze and @jose-luis-rs, thanks for the information. I modified the upexps and seems to work fine. I noticed the timestamp for sofia DAQ is defined differently in unpackers (such as s455) and the R3BSofWhiterabbitReader.cxx. The former names variables as TIMESTAMP_SOFIA_1_WR_T1, while the latter names TIMESTAMP_SOFIA1WR_T1. An example case of the unpacker can be found: |
|
Okay, I was informed that the names of variables are not recommended to have unnecessary underscores in ucesb. Then it makes sense. So ignore the latter comment in the previous post. |
|
Hi @inkdot7 As the current code: R3BRoot/r3bsource/base/R3BUcesbSource.cxx Lines 170 to 183 in 29ff3c6 If we allow some client side items unmatched to the server side items, shouldn't we allow the flag uint32_t map_ok = EXT_DATA_ITEM_MAP_OK | EXT_DATA_ITEM_MAP_NO_DEST | EXT_DATA_ITEM_MAP_NOT_FOUND; |
I think that would undo the entire purpose of this commit - to make sure items are not missing. If items may be missing sometimes, then the client needs to check what was mapped and what was not mapped before use. |
|
I did a test where I use the ext_h101 file generated from the s522 as the client and unpacker from s444 as the server. As you have recently suggested, we should't copy and paste the ext header for each experiment to R3BRoot and instead we should just use one ext headers for as many experiments as possible. In s522, we had 26 planes and triggers are also implemented as well. In s444, we have 16 planes and no trigger. The "array short" flag is expected as we had shortened the array size from 10000 to 1500. However, we get "not found" flags for the signals from plane 17 to plane 26. Our current code forces the program to abort if either "not found" or "array short" is found in any client items. I may misunderstand your suggestion. Or do you actually mean we use the executable from s522 to unpack the data from s444 (both on the server side)? But if so, ext headers are kind of irrelevant. |
|
The suggestion was to only request those items (here planes) which are expected. So each experiment just needs to know how many planes to ask for, and then ask for those items only. That should be most easy with a loop and having made the structure such that e.g. each plane is the structure being asked for. Just like the different detectors are treated separately, one can subdivide further within a detector too. When setting up the request, the plane numbers need to change, so some |
|
Ok, I guess this needs also to be done in runtime. I'm not sure what's the best way to implement this "items that are only asked for". For now, what comes to my mind is something like this: auto struct_map_success = uint32_t{0};
auto status = fClient.setup(NULL, 0, &fStructInfo, &struct_map_success, fEventSize);
if (!status)
{
R3BLOG(fatal, "UCESB error: " << fClient.last_error());
return kFALSE;
}
auto map_ok = EXT_DATA_ITEM_MAP_OK | EXT_DATA_ITEM_MAP_NO_DEST | EXT_DATA_ITEM_MAP_IGNORE;
for(const auto& reader : readers)
{
reader->SetStructInfoIgnore(fStructInfo);
}
auto is_map_ok = CheckIfMapOk(fStructInfo, map_ok);
if(!is_map_ok) { R3BLOG(fatal) << "";}This is just implementation only on R3BRoot side. But in this case |
|
@inkdot7: please do not mention the existence of sprintf. I see two ways forward which I would consider an acceptable solutions (e.g. automatic generation of the h101 preprocessor calls). Given that we already allow the innermost array dimension to be short, I think that we could use a similar approach for the other dimensions. e.g. NeulandReader could require the first plane (or more specifically the first bar of the first plane) to be defined, and everything else is just treated like a short array. One solution would be to look into static reflection libraries for C++. The other solution would be to use an external tool to parse C or C++ (e.g. the onion struct) and autocreate the h101 setup from it. I think either would be an adequate programming challenge. (Bonus points for (a) supporting templated structs like Of course, while overhauling the readers, other stuff to fix would be: One could also debate if ucesb is really the right place for doing the mapping. I would argue that for some purposes, it is impractical to have a full mapping in the ucesb stage. For TAMEX, my understanding is that sixteen channels on a card share one trigger channel, so to abstract away all of the electronics mapping layer would require to duplicate the trigger channels sixteen-fold. (Of course, the practical alternative to doing the mapping in ucesb is storing the mapping in a FairParameter container within a root file. I do not advocate that.) |
|
I had forgotten that the interface to setup the structure However, this I think is not appropriate to use in this case. When e.g. another plane in neuland is expected, its variables need to fully exist. Not only some of them. Letting such things pass would lead to the same problem that was in CALIFA, only in more subtle ways. Thus the suggestion that R3Broot e.g. in the neuland case knows how many planes it should get data for, and it makes exactly the calls to If there are other things that exist for some experiments, and do not for others, that r3broot can also know by some further configuration setting, and then ask or not ask for those items. It would of course be nicer to be able to query what is available before setting up the request, but for the moment that is unfortunately not possible. Or is it? Looks like Also, doing the mapping already in ucesb might not be the better choice. But r3broot can always choose to ask for the data (also) at an earlier unpack levels, or even read the lmd files itself. |
See discussion in #705 .
This patch to exit processing if any requested data members have no source in the input, such that user is alerted and does not get unexpected results.
Please try it out to see what detectors may have issues.