Conversation
|
Hi Laura, is there a reason to make a PDVD customised calibrator? How does it differ from the calibrator which is there already? Wouldn't it be better to have a generic, configurable calibrator that could be used for any ProtoDUNE data and maybe for the far detector, too? |
|
Yes, initially I thought of having a generic calibrator that could be used by all but the PDSP calibrator was created with many hard coded values. I was going to make sure PDSP did not break but I couldn't find a test case for it. So I created a new one. We could change the name to have it generic and still keep the PDSP one. Do you think it would make more sense? |
|
yes, it does make sense. It feels like most of the code (not that there's much) is copied from the SP calibrator. I have made an attempt to make the SP calibrator be more generic in #122. That way, although there are hardcoded numbers specific for SP, they can be over configured from a fhicl. Ideally we would have a generic calibrator with specific configurations for each use case. |
|
Oh, I did not remember about this other PR. Can you check if the new configs that I included here for PDVD work in your module? If so, we can "merge" the two and I will close this one. |
The calibrator here does pretty much the same thing to the one in #122. In there I tried to extend the original SP calibrator so that we would keep the history and compatibility. However, the calibrator here now has the configs for PD VD and I can imagine merging would be tedious. #122 can be dropped. |
| { | ||
|
|
||
| fBadChannels = pset.get<std::vector<int> >("BadChannels"); | ||
| fHdwChannels = pset.get<std::vector<int> >("HdwChannels"); |
There was a problem hiding this comment.
Is fHdwChannels a good name? I know there are different channel definitions and I think what is passed to the calibrator is the "offline channel" number/ID. I don't know if this is the standard naming, but the Hdw prefix makes me thing about the numbering defined by the hardware people.
| fBadChannels = pset.get<std::vector<int> >("BadChannels"); | ||
| fHdwChannels = pset.get<std::vector<int> >("HdwChannels"); | ||
| fSPEAreas = pset.get<std::vector<float> >("SPEAreas"); | ||
| fSPEShifts = pset.get<std::vector<float> >("SPEShifts"); |
There was a problem hiding this comment.
Maybe these fSPEShifts can be empty by default? They would only be applied if they were specified. Thus lines 47 and 48 would be conditional. It would prevent the "expensive" look up in the map if not necessary.
|
|
||
| //Map for associating each hardware channel with its correspondent SPE area | ||
| for(size_t i=0; i<fHdwChannels.size();i++){ | ||
| fAreaMap.emplace(fHdwChannels[i], fSPEAreas[i]); |
There was a problem hiding this comment.
The two maps could be defined as a single map of pairs. This would remove extra lookup.
|
Hi Viktor, I agree with your comments and I think the modifications you included in the calibrator are already a better version of this one in terms of names and the map, no? I am fine with closing this PR as long as the new changes you include in the PDSP calibrator do not break it. Did you test it with old data? We could just include the configs for PDVD in a new or existing fhicl file. |
2 participants
Adding a new calibrator for ProtoDUNE-VD that can handle the different SPE areas for different hardware channels.