Update CalSourceEnergyCalib table format to include ndf; add CalEnergyCalibInfo table

[zwl0331]

This pull request introduces a new calibration metadata table for per-SiPM energy calibration and updates the source calibration table to include additional statistical information. It also ensures these changes are integrated with the table factory for proper instantiation. The main themes are the addition of a new table and enhancements to an existing table.

New calibration metadata table:

• Added CalEnergyCalibInfo class in DbTables/inc/CalEnergyCalibInfo.hh to store combined ADC/MeV calibration constants, uncertainties, and status flags for each SiPM, including handling of different calibration status codes.
• Registered CalEnergyCalibInfo in the table factory by updating DbTables/src/DbTableFactory.cc to include the header and add creation logic for the new table. [1] [2]

Enhancements to source calibration table:

• Extended the CalSourceEnergyCalib::Row class in DbTables/inc/CalSourceEnergyCalib.hh to include a new ndf (number of degrees of freedom) field, and updated all relevant methods, CSV serialization, and constructor to handle this new field. [1] [2] [3] [4] [5]

[FNALbuild]

Hi @zwl0331,
You have proposed changes to files in these packages:

• DbTables

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 31af759: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 31af759.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 31af759 at a500268
build (prof)	✅	Log file. Build time: 04 min 09 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 3 files
clang-tidy	✅	0 errors 0 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 31af759 after being merged into the base branch at a500268.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[rlcee]

Can you describe a bit more what purpose of CalEnergyCalibInfo? The "info" in the name implies it is an "adhoc" table, but it doesn't have the adhoc flag. (example of adhoc is CalCosmicEnergyCalibInfo) It is structured and seems to have the purpose of an archive table, like CalSourceEnergyCalib, and so if the intent is to record, channel by channel, the result of combining the calib sources, I'd expect it to be named more like CalCombinedEnergyCalib to keep in more in parallel with the other archive tables. Thanks

[giro94]

I second Ray's comments. You could call this CalCombinedEnergyCalib and will be essentially identical to the production CalEnergyCalib but with additional information for each row. And, being an archive table, no production maintenance on it. Then, you could think of creating a CalCombinedEnergyCalibInfo adhoc table storing the CIDs of the cosmics and source tables used for the combination (and maybe the CID of the production table?).

[oksuzian]

I'll review this pull request by first loading the PR details and its changes.Let me get the actual file changes in this PR.Now let me look at the existing similar table (CalCosmicEnergyCalibInfo) for comparison, and the original CalSourceEnergyCalib to understand the full context:Now I have a thorough understanding of the PR. Let me provide my review.

---

PR Review: #1718

This PR makes two changes: (1) adds an ndf field to CalSourceEnergyCalib, and (2) adds a new CalEnergyCalibInfo table. I found several issues ranging from a bug to style/design concerns.

---

🐛 Bug: Pre-existing rowToCsv bug fixed, but fix is incomplete

The original CalSourceEnergyCalib::rowToCsv on main was already buggy — it was missing the frFull, frFirst, and frSecond fields. It jumped from secescErrWidth directly to chisq:

    void rowToCsv(std::ostringstream& sstream, std::size_t irow) const override {
      Row const& r = _rows.at(irow);
      sstream << std::fixed << std::setprecision(5);
      sstream << r.roid()<<",";
      sstream << r.fullEPeak()<<",";
      ...
      sstream << r.secescErrWidth()<<",";
      sstream << r.chisq();  // <-- frFull, frFirst, frSecond are missing!
    }

This PR fixes that pre-existing bug by adding the missing fields. This is good, but it should be explicitly mentioned in the PR description as a bug fix, since it changes the CSV output format independently of the ndf addition. Consumers of this CSV data will see a format change.

---

⚠️ Issue 1: status_message with commas will break CSV parsing

In CalEnergyCalibInfo, the status_message field is a free-form std::string written directly to CSV without any quoting/escaping:

    sstream << r.status_code() << ",";
    sstream << r.status_message();

And in addRow, it's read as simply columns[4]:

    _rows.emplace_back(CaloSiPMId(index), std::stof(columns[1]),
                       std::stof(columns[2]), std::stoi(columns[3]),
                       columns[4]);

If status_message ever contains a comma, CSV parsing will break — the message will be split across multiple column indices. Since this is the last column it partially mitigates the issue (the base DbTable parser would need to handle it), but it's still fragile. Consider:

• Documenting that commas are forbidden in status_message, or
• Using a fixed enum/int instead of a free-form string, or
• Ensuring the DbTable CSV parser treats the last column as "rest of line"

---

⚠️ Issue 2: Narrowing conversion in addRow — std::stoul to std::uint16_t

    std::uint16_t index = std::stoul(columns[0]);

std::stoul returns unsigned long, which is silently narrowed to uint16_t. If the parsed value exceeds 65535, it will silently wrap. This is the same pattern used in the existing CalSourceEnergyCalib, so it's consistent with the codebase, but worth noting. The subsequent bounds check against CaloConst::_nChannelDB does provide a safety net.

---

⚠️ Issue 3: std::string status_message passed by value in constructor

    Row(CaloSiPMId roid, float ADC2MeV, float ADC2MeV_err,
        int status_code, std::string status_message)
        : _roid(roid),
          _ADC2MeV(ADC2MeV),
          _ADC2MeV_err(ADC2MeV_err),
          _status_code(status_code),
          _status_message(status_message) {}

The status_message parameter is taken by value but then copied into _status_message. It should either be taken by const std::string& (to avoid one extra copy) or moved via std::move(status_message) in the initializer list. For example:

_status_message(std::move(status_message)) {}

This also applies to the existing CalCosmicEnergyCalibInfo (same pattern), but for new code it's worth getting right.

---

💡 Issue 4: std::setprecision(5) applied to integer fields

In CalEnergyCalibInfo::rowToCsv:

    sstream << std::fixed << std::setprecision(5);
    sstream << r.roid() << ",";
    sstream << r.ADC2MeV() << ",";
    sstream << r.ADC2MeV_err() << ",";
    sstream << r.status_code() << ",";
    sstream << r.status_message();

std::fixed with setprecision(5) doesn't affect integer output, but it does affect roid() if CaloSiPMId's operator<< outputs a float-like value. This is consistent with the codebase pattern, so it's minor.

---

💡 Issue 5: Inconsistent coding style between the new file and existing codebase

The new CalEnergyCalibInfo.hh uses a different formatting style (indentation, brace placement, spacing) compared to the existing tables like CalSourceEnergyCalib.hh and CalCosmicEnergyCalibInfo.hh. For example:

• New file uses 2-space indent with braces on same line (formatted style)
• Existing files use mixed indentation with more compact formatting

This is a minor consistency point but worth aligning for maintainability.

---

💡 Issue 6: Missing #include <vector> in CalEnergyCalibInfo.hh

The new file includes <vector> explicitly, which is good. However, it does not include "Offline/DataProducts/inc/CaloConst.hh" — wait, it does include it. And it doesn't include <map> like CalSourceEnergyCalib.hh does (though <map> isn't actually used there either). This is fine.

---

Summary

Severity	Issue	Location
🐛 Bug (pre-existing, fixed here)	rowToCsv was missing frFull, frFirst, frSecond fields — this is a behavior change that should be documented	CalSourceEnergyCalib.hh
⚠️ Medium	status_message string in CSV can break parsing if it contains commas	CalEnergyCalibInfo.hh
⚠️ Low	std::string passed by value instead of by const-ref or moved	CalEnergyCalibInfo.hh Row constructor
💡 Nit	Inconsistent code style with existing tables	CalEnergyCalibInfo.hh

The most important item to address is Issue 1 (the status_message CSV safety concern). The pre-existing rowToCsv bug fix is a welcome improvement but should be called out in the PR description since it changes behavior beyond just adding ndf.

[FNALbuild]

📝 The HEAD of main has changed to 10120d9. Tests are now out of date.

[zwl0331]

Can you describe a bit more what purpose of CalEnergyCalibInfo? The "info" in the name implies it is an "adhoc" table, but it doesn't have the adhoc flag. (example of adhoc is CalCosmicEnergyCalibInfo) It is structured and seems to have the purpose of an archive table, like CalSourceEnergyCalib, and so if the intent is to record, channel by channel, the result of combining the calib sources, I'd expect it to be named more like CalCombinedEnergyCalib to keep in more in parallel with the other archive tables. Thanks

The purpose of it is to store extra information from the combination algorithm such as the error on the ADCtoMeV constant and the source of this constant (such as "only using cosmic" or "averaging cosmic and source"). I have flagged it as an adhoc table.

I named it a "CalEnergyCalibInfo" table because it is meant to be a table to complement the information in the "CalEnergyCalib" table, just like how "CalCosmicEnergyCalibInfo" is meant to complement "CalCosmicEnergyCalib" table.

[oksuzian]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for 71d1484: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at 71d1484.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 71d1484 at 3e5e0e6
build (prof)	✅	Log file. Build time: 04 min 11 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 4 files
clang-tidy	✅	0 errors 0 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 71d1484 after being merged into the base branch at 3e5e0e6.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[giro94]

I have flagged it as an adhoc table.

@zwl0331 adhoc tables work a bit differently as each commit is added as a row. We should make this a standard archive table (which has nothing different wrt a production table except for how we commit it to the database).
I propose we could add another ad-hoc table to be able to link the CIDs of CalEnergyCalib and CalEnergyCalibInfo.
Can we discuss this on Friday?