Hcal tot digitization #1475
Hcal tot digitization #1475
Conversation
There was a problem hiding this comment.
Thank you for getting this started!
Generally, I would like to see some more detailed explanation on what you've done and then some plots showing that your developments are doing what is intended. Additionally, you have edited HgcrcoEmulator which is also used within the Ecal digitiziation pipeline. Have you confirmed that these edits do not change that pipeline at all?
(Side note: remove the .DS_Store files from the branch (using git rm). These are not related to the software and just tells me that you are on MacOS!)
|
New Class 'PulseRecord': This class is intended solely for plotting purposes and should not interfere with the existing workflow. The PulseRecord(double volts, int adc, int tot) function temporarily stores the digitized pulses when the HgcrocEmulator is called by either the Hcal or Ecal pipelines and is cleared each time the emulator is invoked. If the pulse is digitized using ADC, the TOT value is set to zero, and vice versa. To my understanding, the HgcrocEmulator cannot be called by the Ecal and Hcal pipelines at the exact same time. Therefore, when the getter for PulseRecord is called within the HcalDigiProducer, it will only retrieve the digitized information corresponding to the pulses sent for digitization at that moment. If this understanding is incorrect, the class may only be suitable for studying the back Hcal. HgcrocEmulator HgcrocEmulator.h has been amended with a mutable vector ‘pulseRecord_’ and a getter to access ‘pulseRecord_’ to store and access digitizations. Originally, the TOT digitization for an incoming pulse was set by comparing the voltage (vpeak) of the hit, calculated using the pulse function at the time of the hit that comes with the pulse. However, in ADC digitization, the voltage (bxvolts) is calculated using iADC and the clock cycle. There is a slight difference between the two voltage values. See the following debug print. 
If we used the original setting: if (vpeak > totThreshold) { ADC and TOT digitizations look like this. Note that the ADC digitization does not reach up to 1024 counts here.
Instead, if we modify the code as follows, the complete ADC range is utilized in the digitization:
In this branch, the ‘bxvolts > totThreshold’ condition is currently used. HcalDigiProducer Once each section in HcalDigiProducer sends pulses to HgcrocEmulator, they are accessed through the getter and temporarily stored in the vector ‘DigiToPlot’, which is cleared for each ‘hitsByID’ in the loop. At the end of each section, the recorded digitized info is written into a text file following the format [Voltage ADC TOT]. The plots I showed above were generated using that text file. hcal_hardcoded_conditions.py and proposed TOT reconstruction Please see the following presentation to understand how changes were made to hcal_hardcoded_conditions and how TOT reconstruction can be implemented. Discussion Before the implementation of TOT digitization, the histogram of Hcal reconstructed energy was piling up towards the last bins. However, after the implementation, we now observe a smoothed decay in the reconstructed energy. Please note that the appropriate parameters for reconstruction have not yet been configured.
|
There was a problem hiding this comment.
Thank you for the extra detail. I agree that the new PulseRecord class would not be affecting the emulation. The HgcrocEmulator not only is called at separate times but the Ecal and Hcal subsystems have their own copies of the class so they will not interfere. My concern about how this affects the Ecal emulation is from the change in the value used to determine if a specific pulse is above the tot threshold or not. I've highlighted the specific code below. This change is well motivated, I just want to understand the downstream effects.
Besides more specific comments below, my other requests are to remove the .DS_Store files that are not interesting for us and write some more documentation into the PulseRecord header file. You've already written a nice summary in your comment above so that is a good place to start.
| // Print Voltages, adc/tot values to the same txt file | ||
| std::ofstream dataFile("Voltage_ADC_TOT.txt", std::ios::app); | ||
| for (const auto& entry : DigiToPlot) { | ||
| dataFile << std::get<0>(entry) << " " << std::get<1>(entry) << " " | ||
| << std::get<2>(entry) << "\n"; | ||
| } | ||
| dataFile.close(); |
There was a problem hiding this comment.
This chunk of code (and the others like it) is incredibly helpful for you the developer when tuning the digitization parameters; however, most people running this producer will not be tuning the parameters and creating an extra txt file would at best be confusing and at worst cause extra issues.
My request here is to put these code blocks behind a configuration parameter such that the user can choose whether to save the digitization information. Maybe a digi_emulation_file parameter that is the file to save the information to? (e.g. it could be "Voltage_ADC_TOT.txt") and when it is empty, nothing is saved.
| for (const auto& record : pulseRecord) { | ||
| DigiToPlot.emplace_back(record.getVolts(), record.getADC(), | ||
| record.getTOT()); | ||
| } |
There was a problem hiding this comment.
One of the code blocks that could be skipped if the user doesn't want to save the digi emulation detailed information.
| // Recording digitized pulses in PulseRecord | ||
| // Access PulseRecord | ||
| const auto& pulseRecord = hgcroc_->getPulseRecord(); | ||
|
|
||
| for (const auto& record : pulseRecord) { | ||
| DigiToPlot.emplace_back(record.getVolts(), record.getADC(), | ||
| record.getTOT()); | ||
| } | ||
|
|
||
| // Print Voltages, adc/tot values to the same txt file | ||
| std::ofstream dataFile("Voltage_ADC_TOT.txt", std::ios::app); | ||
| for (const auto& entry : DigiToPlot) { | ||
| dataFile << std::get<0>(entry) << " " << std::get<1>(entry) << " " | ||
| << std::get<2>(entry) << "\n"; | ||
| } | ||
| dataFile.close(); | ||
|
|
There was a problem hiding this comment.
One of the code blocks that could be skipped if the user doesn't want to save the digi emulation detailed information.
|
|
||
| // Recording digitized pulses in PulseRecord | ||
| // Access PulseRecord | ||
| const auto& pulseRecord = hgcroc_->getPulseRecord(); | ||
|
|
||
| for (const auto& record : pulseRecord) { | ||
| DigiToPlot.emplace_back(record.getVolts(), record.getADC(), | ||
| record.getTOT()); | ||
| } | ||
|
|
||
| // Print Voltages, adc/tot values to the same txt file | ||
| std::ofstream dataFile("Voltage_ADC_TOT.txt", std::ios::app); | ||
| for (const auto& entry : DigiToPlot) { | ||
| dataFile << std::get<0>(entry) << " " << std::get<1>(entry) << " " | ||
| << std::get<2>(entry) << "\n"; | ||
| } | ||
| dataFile.close(); |
There was a problem hiding this comment.
One of the code blocks that could be skipped if the user doesn't want to save the digi emulation detailed information.
|
|
||
| if (vpeak > totThreshold) { | ||
| if (bxvolts > totThreshold) { |
There was a problem hiding this comment.
This is the part that I am worried about effecting the ECal digi emulation as well. This is a substantial change and could affect things there and, while the change is well motivated, I would like to understand what the results are.
| return true; // always readout | ||
| for (const auto &digi : digiToAdd) return true; // always readout | ||
|
|
There was a problem hiding this comment.
You don't need the for loop. It will exit on the first entry in the for loop anyways.
| return true; // always readout | |
| for (const auto &digi : digiToAdd) return true; // always readout | |
| return true; // always readout |
I am updating ldmx-sw, with implemented Hcal TOT digitization.
What are the issues that this addresses?
Check List