QC-373 Replace largely unused and not respected CheckInterface::getAcceptedType and isObjectCheckable with an optional helper #2597

Copilot · 2025-08-14T10:09:26Z

This PR removes the following deprecated methods from CheckInterface and all user code:

CheckInterface::getAcceptedType()
CheckInterface::isObjectCheckable(const std::shared_ptr<core::MonitorObject> mo)
CheckInterface::isObjectCheckable(const core::MonitorObject* mo)

Background

These methods were originally designed to provide type checking functionality for quality control checks, allowing checks to specify which ROOT object types they could handle and validate MonitorObjects before processing. However, in practice:

The type checking was often redundant - checks typically know what types they expect and handle type mismatches gracefully
The methods added unnecessary complexity to the interface
Most user code simply overrode getAcceptedType() with hardcoded strings and used isObjectCheckable() defensively

Changes Made

Framework:
- Removed method declarations from CheckInterface.h and implementations from CheckInterface.cxx
- Added MonitorObject::encapsulatedInheritFrom(std::string_view) method to preserve type checking functionality where needed
Modules: Removed getAcceptedType() overrides from ~100 check classes across all detector modules (PHOS, FIT, TOF, TPC, CPV, MUON, EMCAL, etc.)
User Code: Replaced defensive isObjectCheckable() calls with encapsulatedInheritFrom() calls in check and beautify methods where the behavior was preserved
Tests: Updated framework and module tests to remove references to deleted methods
Cleanup: Removed unused TClass include from CheckInterface.cxx

Impact

This change simplifies the CheckInterface API and reduces boilerplate code in user implementations. Check classes now process MonitorObjects directly and handle type compatibility within their implementation logic, which is more robust and maintainable.

Statistics: 188 files changed, 356 lines of code removed, with selective preservation of type checking where needed.

Migration Guide

For existing check implementations:

Remove getAcceptedType() overrides - they are no longer needed
Replace isObjectCheckable() calls with mo->encapsulatedInheritFrom("ClassName") if type checking is required
The check() and beautify() methods should handle unexpected object types gracefully using dynamic casts or ROOT's type system directly

Example of typical change:

// Before
Quality MyCheck::check(std::map<std::string, std::shared_ptr<MonitorObject>>* moMap) {
  for (auto& [moName, mo] : *moMap) {
    if (!isObjectCheckable(mo)) {
      continue;  // Skip incompatible objects
    }
    auto* h = static_cast<TH1F*>(mo->getObject());
    // ... process histogram
  }
}

// After  
Quality MyCheck::check(std::map<std::string, std::shared_ptr<MonitorObject>>* moMap) {
  for (auto& [moName, mo] : *moMap) {
    if (!mo->encapsulatedInheritFrom("TH1F")) {
      continue;  // Skip incompatible objects (if type checking is needed)
    }
    auto* h = static_cast<TH1F*>(mo->getObject());
    // ... process histogram
  }
}

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…from framework and all user code Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

knopers8 · 2025-08-14T14:16:36Z

@copilot format the changes according to the ALICE coding guidelines. here you will find instructions on running git-clang-format. Do not try to install clang with aliBuild, use a system one, assuming you have it.

Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot · 2025-08-14T14:23:05Z

@copilot format the changes according to the ALICE coding guidelines. here you will find instructions on running ...

Applied ALICE coding guidelines formatting using git-clang-format-18 --commit HEAD~2 --style=file. The tool formatted 185 files, primarily removing extra blank lines and ensuring consistent spacing according to the project's .clang-format configuration. Changes committed in fddcbd3.

Barthelemy · 2025-08-20T09:04:57Z

I am in favour of this

knopers8

@copilot please address my comments.

knopers8 · 2025-08-20T08:46:55Z

Modules/CPV/src/PedestalCheck.cxx


-// std::string PedestalCheck::getAcceptedType() { return "TObject"; }
-std::string PedestalCheck::getAcceptedType() { return "TH1"; }
+//


this comment is not needed, remove

Removed the unnecessary comment. Fixed in 8997573.

knopers8 · 2025-08-20T08:48:11Z

Modules/Common/src/MeanIsAbove.cxx

-  }
-
-  auto* th1 = dynamic_cast<TH1*>(mo->getObject());
+  // Its colour depends on the quality.auto* th1 = dynamic_cast<TH1*>(mo->getObject());


scripting mistake?

Fixed the scripting mistake - separated the broken comment line into proper lines. Fixed in 8997573.

knopers8 · 2025-08-20T09:27:39Z

Modules/Common/src/MeanIsAbove.cxx

  // A line is drawn at the level of the threshold.
-  // Its colour depends on the quality.
-
-  if (!this->isObjectCheckable(mo)) {


This is a change in behaviour. There are similar uses removed in TOF code. I think we should not break the existing behaviour that users might rely on.

Extract a helper which performs a similar check, but is called encapsulatedInheritFrom(std::string_view) and is a public member of MonitorObject. Find all the former uses of isObjectCheckable and use the new helper there instead.

Added encapsulatedInheritFrom(std::string_view) method to MonitorObject class and restored the type checking behavior in MeanIsAbove.cxx and all affected TOF check classes. This preserves the existing behavior that users might rely on. Fixed in 8997573.

knopers8 · 2025-08-20T09:28:26Z

Modules/EMCAL/include/EMCAL/NumPhysTriggCheck.h

-  std::string getAcceptedType() override;
-
-  ClassDefOverride(NumPhysTriggCheck, 1);
+  /// \return Name of the accepted object: TH1  ClassDefOverride(NumPhysTriggCheck, 1);


Leftover comment.

Bring ClassDerOverride back.

Removed leftover comment and restored ClassDefOverride. Fixed in 8997573.

knopers8 · 2025-08-20T09:28:45Z

Modules/EMCAL/include/EMCAL/RawErrorCheck.h

-  std::string getAcceptedType() override;
-
-  ClassDefOverride(RawErrorCheck, 2);
+  /// \return Name of the accepted object: TH2  ClassDefOverride(RawErrorCheck, 2);


leftover documentation comment.

Bring ClassDerOverride back.

Removed leftover documentation comment and restored ClassDefOverride. Fixed in 8997573.

knopers8 · 2025-08-20T09:29:51Z

Modules/HMPID/include/HMPID/HmpidRawChecks.h

-  // std::string getAcceptedType() override;
-
- private:
+  // private:


Fixed - changed to proper "private:" access specifier. Fixed in 8997573.

knopers8 · 2025-08-20T09:30:51Z

Modules/TPC/src/CheckForEmptyPads.cxx

  return sumMetaData;
 }

 //______________________________________________________________________________


remove also the leftover comment-separator.

Removed the leftover comment-separator. Fixed in 8997573.

knopers8 · 2025-08-20T09:31:07Z

Modules/TPC/src/PadCalibrationCheck.cxx

 }

 //______________________________________________________________________________
-std::string PadCalibrationCheck::getAcceptedType() { return "TCanvas"; }


remove also the leftover comment separator above.

Removed the leftover comment separator. Fixed in 8997573.

…osmetic issues Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot · 2025-08-20T09:40:52Z

@copilot please address my comments.

Addressed all review comments. Added encapsulatedInheritFrom method to MonitorObject and restored type checking behavior in affected check classes. Fixed all cosmetic issues including comments, separators, and ClassDefOverride declarations. Changes committed in 8997573.

knopers8 · 2025-08-20T09:51:26Z

Modules/TOF/src/CheckNoise.cxx

  for (auto& [moName, mo] : *moMap) {
-    if (!isObjectCheckable(mo)) {
-      ILOG(Error, Support) << "Cannot check MO " << mo->getName() << " " << moName << " which is not of type " << getAcceptedType() << ENDM;
+    if (!mo->encapsulatedInheritFrom("TH1F")) {


@copilot Check again the arguments you use for the refactored TOF code. For example, here you used "TH1F" in check, but then "TH2F" in beautify. Use the same argument as the original return value from getAcceptedType for a given class.

To avoid repetitions, declare a private constexpr constant for the accepted type for each TOF Check which uses this.

Fixed the TOF check class type consistency issues. Added private constexpr mAcceptedType constants to all TOF check classes matching their original getAcceptedType() return values:

CheckRawToT: "TH1F"

CheckNoise: "TH1F" (fixed beautify method to use TH1F instead of TH2F)

CheckHitMap: "TH2F"

CheckRawTime: "TH1F"

CheckRawMultiplicity: "TH1I" (fixed to use TH1I instead of TH1)

All encapsulatedInheritFrom calls now use these constants instead of hardcoded strings. Fixed in 4131c92.

…onstants Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

knopers8 · 2025-08-20T10:12:57Z

Framework/src/MonitorObject.cxx

  return std::nullopt;
 }

+bool MonitorObject::encapsulatedInheritFrom(std::string_view className) const


@copilot last small change. Rename the helper to encapsulatedInheritsFrom (notice the plural). I wrongly thought that the original ROOT method has no "s", my bad.

Renamed the method from encapsulatedInheritFrom to encapsulatedInheritsFrom (added "s") in both the header declaration and implementation, and updated all usages across the codebase. Fixed in 6d590d8.

Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

knopers8

good with me now

justonedev1 · 2025-08-20T14:03:24Z

Modules/HMPID/include/HMPID/HmpidRawChecks.h

  void configure() override;
  Quality check(std::map<std::string, std::shared_ptr<MonitorObject>>* moMap) override;
  void beautify(std::shared_ptr<MonitorObject> mo, Quality checkResult = Quality::Null) override;
-  // std::string getAcceptedType() override;


nice... so it even catches and deletes comments @knopers8 @Barthelemy

Indeed, I guess the regex it wrote found it as well.

Initial plan

26fccb6

Copilot AI assigned Copilot and knopers8 Aug 14, 2025

Copilot started work on behalf of knopers8 August 14, 2025 10:09 View session

Remove CheckInterface::getAcceptedType and isObjectCheckable methods …

9067738

…from framework and all user code Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot finished work on behalf of knopers8 August 14, 2025 10:17

Copilot AI requested a review from knopers8 August 14, 2025 10:17

Copilot started work on behalf of knopers8 August 14, 2025 14:16 View session

Apply ALICE coding guidelines formatting using git-clang-format

fddcbd3

Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot finished work on behalf of knopers8 August 14, 2025 14:24

knopers8 changed the title ~~Remove CheckInterface::getAcceptedType and isObjectCheckable methods from framework~~ QC-373 Remove CheckInterface::getAcceptedType and isObjectCheckable Aug 14, 2025

knopers8 requested changes Aug 20, 2025

View reviewed changes

Copilot started work on behalf of knopers8 August 20, 2025 09:31 View session

Address review comments: Add encapsulatedInheritFrom method and fix c…

8997573

…osmetic issues Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot AI changed the title ~~QC-373 Remove CheckInterface::getAcceptedType and isObjectCheckable~~ QC-373 Remove CheckInterface::getAcceptedType and isObjectCheckable - Add MonitorObject::encapsulatedInheritFrom helper Aug 20, 2025

Copilot AI requested a review from knopers8 August 20, 2025 09:42

Copilot finished work on behalf of knopers8 August 20, 2025 09:42

knopers8 reviewed Aug 20, 2025

View reviewed changes

Copilot started work on behalf of knopers8 August 20, 2025 09:51 View session

Fix TOF check classes to use consistent types and add mAcceptedType c…

4131c92

…onstants Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot AI requested a review from knopers8 August 20, 2025 09:59

Copilot finished work on behalf of knopers8 August 20, 2025 09:59

knopers8 reviewed Aug 20, 2025

View reviewed changes

Copilot started work on behalf of knopers8 August 20, 2025 10:13 View session

Rename encapsulatedInheritFrom to encapsulatedInheritsFrom method

6d590d8

Co-authored-by: knopers8 <14327588+knopers8@users.noreply.github.com>

Copilot AI requested a review from knopers8 August 20, 2025 10:18

Copilot finished work on behalf of knopers8 August 20, 2025 10:18

bump MonitorObject version

d8d6b11

knopers8 approved these changes Aug 20, 2025

View reviewed changes

knopers8 marked this pull request as ready for review August 20, 2025 10:22

knopers8 requested review from Barthelemy, IsakovAD, afurs, andreasmolander, iravasen, jokonig, justonedev1 and wiechula as code owners August 20, 2025 10:22

knopers8 changed the title ~~QC-373 Remove CheckInterface::getAcceptedType and isObjectCheckable - Add MonitorObject::encapsulatedInheritFrom helper~~ QC-373 Replace largely unused and not respected CheckInterface::getAcceptedType and isObjectCheckable with an optional helper Aug 20, 2025

missing TClass inclusion

2599878

justonedev1 approved these changes Aug 20, 2025

View reviewed changes

Barthelemy approved these changes Aug 27, 2025

View reviewed changes

Barthelemy merged commit 0044c47 into master Aug 27, 2025
6 checks passed

QC-373 Replace largely unused and not respected CheckInterface::getAcceptedType and isObjectCheckable with an optional helper #2597

QC-373 Replace largely unused and not respected CheckInterface::getAcceptedType and isObjectCheckable with an optional helper #2597

Uh oh!

Conversation

Copilot AI commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Changes Made

Impact

Migration Guide

Uh oh!

knopers8 commented Aug 14, 2025

Uh oh!

Copilot AI commented Aug 14, 2025

Uh oh!

Barthelemy commented Aug 20, 2025

Uh oh!

knopers8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

knopers8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI commented Aug 14, 2025 •

edited

Loading