Skip to content

do not abort for missing trms calib#4148

Merged
pinkenburg merged 1 commit intosPHENIX-Collaboration:masterfrom
pinkenburg:mbd-hotfix
Jan 30, 2026
Merged

do not abort for missing trms calib#4148
pinkenburg merged 1 commit intosPHENIX-Collaboration:masterfrom
pinkenburg:mbd-hotfix

Conversation

@pinkenburg
Copy link
Contributor

@pinkenburg pinkenburg commented Jan 30, 2026

Types of changes

  • [ X] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

The latest change made a missing calibration a fatal error. The MBD_TRMS calibration does not exist so the reconstruction aborts now. This is a quick fix which leaves everything in place except the abort if this calibration is not found. The reconstruction now goes through (means the other calibrations do exist, will merge this right after jenkins kicks off. clang-tidy will fail since some includes have changed which are not part of the new build yet.

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

PR Summary

Motivation / Context

A recent change (commit f493c6e) made missing MBD calibrations fatal errors to prevent reconstruction with incomplete data. However, the MBD_TRMS (time resolution vs. amplitude) calibration does not yet exist in the database. This caused reconstruction to abort when the TRMS calibration was unavailable, even though the other required calibrations were present. This PR provides a temporary fix to allow reconstruction to proceed when TRMS is missing while more fundamental calibration infrastructure is developed.

Key Changes

  • Download_TimeRMS() now returns 0 (success) instead of -1 (failure) when the time RMS calibration data is missing
    • Commented out the error status assignment (_status = -1) and early return that previously caused reconstruction to abort
    • A WARNING message is still logged when the calibration is missing
    • Minor formatting: removed a stray blank line

Potential Risk Areas

  1. Reconstruction behavior change: Reconstruction now proceeds when TRMS is missing, rather than aborting. This is intentional but assumes other calibrations are sufficient for physics quality.

  2. Status inconsistency: Unlike other missing calibrations that set _status = -1 and cause failure, TRMS now returns 0 without explicitly setting status. This creates an inconsistency in error handling across different calibration types.

  3. Silent degradation: The function logs a WARNING but doesn't prevent potential downstream issues if TRMS-dependent analysis is performed without the calibration data. Downstream code should verify TRMS availability when needed.

Possible Future Improvements

  • Create and populate the MBD_TRMS calibration database to eliminate this exception
  • Implement a more explicit optional calibration mechanism rather than special-casing individual calibrations
  • Add validation in downstream code that depends on TRMS data to verify its availability
  • Consider a formal "required vs. optional calibration" framework to clarify which calibrations must be present

Note: AI-generated summaries may contain errors. Please verify against actual code changes when reviewing.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

Modifies MbdCalib::Download_TimeRMS to return 0 instead of -1 when time RMS calibration data is unavailable, and removes the associated error status assignment. Includes minor whitespace cleanup.

Changes

Cohort / File(s) Summary
MBD Calibration Time RMS Handling
offline/packages/mbd/MbdCalib.cc
Changed Download_TimeRMS to return 0 (non-error) instead of -1 when _trms_y[0] is empty, and commented out the error status assignment. This alters the function's signaling behavior for missing TRMS data. Minor formatting: removed blank line after _trms_npts reset.

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

Tip

🧪 Unit Test Generation v2 is now available!

We have significantly improved our unit test generation capabilities.

To enable: Add this to your .coderabbit.yaml configuration:

reviews:
  finishing_touches:
    unit_tests:
      enabled: true

Try it out by using the @coderabbitai generate unit tests command on your code files or under ✨ Finishing Touches on the walkthrough!

Have feedback? Share your thoughts on our Discord thread!


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pinkenburg
Copy link
Contributor Author

There will still be a (not fatal) message that the trms calibration is missing (to remind us that something needs to be done here)

@pinkenburg pinkenburg merged commit 802d4d2 into sPHENIX-Collaboration:master Jan 30, 2026
6 of 9 checks passed
@sphenix-jenkins-ci
Copy link

Build & test report

Report for commit 3ab3316d1593fc99b5e70a704614c0e95b743eaf:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg pinkenburg deleted the mbd-hotfix branch January 30, 2026 18:41
@coderabbitai coderabbitai bot mentioned this pull request Feb 1, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant