Skip to content

FairModule IsSensitive checks medium property not volume name #1628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rlalik wants to merge 3 commits into from
Closed

FairModule IsSensitive checks medium property not volume name #1628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2895ace
refactor(base): FairModule IsSensitive checks medium property not vol…
rlalik Mar 27, 2026
c700986
docs(CONTRIBUTORS): Update contributor records
rlalik Mar 28, 2026
b3c2c2a
refactor(base): Enforce new sensitivity model on dependend code
rlalik Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .zenodo.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@
"type": "Other",
"name": "Krzewicki, Mikolaj"
},
{
"type": "Other",
"name": "Lalik, Rafa\u0142",
"orcid": "0000-0003-1313-3729"
},
{
"type": "Other",
"name": "Lavezzi, Lia"
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Klenze, Philipp
Kollegger, Thorsten
Koenig, Ilse
Krzewicki, Mikolaj
Lalik, Rafał [https://orcid.org/0000-0003-1313-3729]
Lavezzi, Lia
Lavrik, Evgeny
Lantwin, Oliver
Expand Down
6 changes: 6 additions & 0 deletions codemeta.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@
"givenName": "Mikolaj",
"familyName": "Krzewicki"
},
{
"@type": "Person",
"@id": "https://orcid.org/0000-0003-1313-3729",
"givenName": "Rafa\u0142",
"familyName": "Lalik"
},
{
"@type": "Person",
"givenName": "Lia",
Expand Down
5 changes: 0 additions & 5 deletions examples/MQ/pixelDetector/src/Pixel.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,6 @@ void Pixel::ConstructGeometry()
ConstructASCIIGeometry<PixelGeo, PixelGeoPar>("PixelGeoPar");
}

Bool_t Pixel::IsSensitive(const std::string& name)
{
return name.find("Pixel") != std::string::npos;
}

PixelPoint* Pixel::AddHit(Int_t trackID,
Int_t detID,
TVector3 pos,
Expand Down
1 change: 0 additions & 1 deletion examples/MQ/pixelDetector/src/Pixel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ class Pixel : public FairDetector

void EndOfEvent() override;

Bool_t IsSensitive(const std::string& name) override;
FairModule* CloneModule() const override;

private:
Expand Down
6 changes: 0 additions & 6 deletions examples/common/passive/FairMagnet.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ void FairMagnet::ConstructGeometry()
}
}

Bool_t FairMagnet::IsSensitive(const std::string& /*name*/)
{
// just to get rid of the warrning during run, not need this is a passive element!
return kFALSE;
}

void FairMagnet::ConstructASCIIGeometry()
{
FairModule::ConstructASCIIGeometry<FairGeoMagnet, FairGeoPassivePar>("FairGeoPassivePar");
Expand Down
1 change: 0 additions & 1 deletion examples/common/passive/FairMagnet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class FairMagnet : public FairModule
virtual ~FairMagnet();
void ConstructGeometry();
void ConstructASCIIGeometry();
Bool_t IsSensitive(const std::string& name);

virtual FairModule* CloneModule() const;

Expand Down
2 changes: 0 additions & 2 deletions examples/simulation/Tutorial1/src/FairFastSimExample.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,6 @@ FairTutorialDet1Point* FairFastSimExample::AddHit(Int_t trackID,
return new (clref[size]) FairTutorialDet1Point(trackID, detID, pos, mom, time, length, eLoss);
}

Bool_t FairFastSimExample::IsSensitive(const std::string& name) { return name == "fast_sim_vol"; }

FairModule* FairFastSimExample::CloneModule() const
{
return new FairFastSimExample(*this);
Expand Down
2 changes: 0 additions & 2 deletions examples/simulation/Tutorial1/src/FairFastSimExample.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ class FairFastSimExample : public FairFastSimDetector
*/
virtual void EndOfEvent();

virtual Bool_t IsSensitive(const std::string& name);

virtual FairModule* CloneModule() const;

protected:
Expand Down
2 changes: 0 additions & 2 deletions examples/simulation/Tutorial1/src/FairFastSimExample2.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ FairTutorialDet1Point* FairFastSimExample2::AddHit(Int_t trackID,
return new (clref[size]) FairTutorialDet1Point(trackID, detID, pos, mom, time, length, eLoss);
}

Bool_t FairFastSimExample2::IsSensitive(const std::string& name) { return name == "fast_sim_vol_n2"; }

FairModule* FairFastSimExample2::CloneModule() const
{
return new FairFastSimExample2(*this);
Expand Down
2 changes: 0 additions & 2 deletions examples/simulation/Tutorial1/src/FairFastSimExample2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ class FairFastSimExample2 : public FairFastSimDetector
*/
virtual void EndOfEvent();

virtual Bool_t IsSensitive(const std::string& name);

virtual FairModule* CloneModule() const;

protected:
Expand Down
5 changes: 0 additions & 5 deletions examples/simulation/Tutorial1/src/FairTutorialDet1.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ TClonesArray* FairTutorialDet1::GetCollection(Int_t iColl) const

void FairTutorialDet1::Reset() { fFairTutorialDet1PointCollection->Clear(); }

Bool_t FairTutorialDet1::IsSensitive(const std::string& name)
{
return name.find("tutdet") != std::string::npos;
}

void FairTutorialDet1::ConstructGeometry()
{
/** If you are using the standard ASCII input for the geometry
Expand Down
2 changes: 0 additions & 2 deletions examples/simulation/Tutorial1/src/FairTutorialDet1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,6 @@ class FairTutorialDet1 : public FairDetector

FairModule* CloneModule() const override;

Bool_t IsSensitive(const std::string& name) override;

private:
/** Track information to be stored until the track leaves the
active volume.
Expand Down
5 changes: 0 additions & 5 deletions examples/simulation/Tutorial4/src/mc/FairTutorialDet4.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,6 @@ void FairTutorialDet4::ConstructGeometry()
}
}

Bool_t FairTutorialDet4::IsSensitive(const std::string& name)
{
return name.find("tut4") != std::string::npos;
}

void FairTutorialDet4::ConstructASCIIGeometry()
{
/** If you are using the standard ASCII input for the geometry
Expand Down
2 changes: 0 additions & 2 deletions examples/simulation/Tutorial4/src/mc/FairTutorialDet4.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ class FairTutorialDet4 : public FairDetector

virtual void RegisterAlignmentMatrices();

virtual Bool_t IsSensitive(const std::string& name);

private:
/** Track information to be stored until the track leaves the
active volume.
Expand Down
2 changes: 1 addition & 1 deletion fairroot/base/sim/FairDetector.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ void FairDetector::DefineSensitiveVolumes()
TIter next(volumes);
TGeoVolume* volume;
while ((volume = static_cast<TGeoVolume*>(next()))) {
if (IsSensitive(volume->GetName())) {
if (IsSensitive(volume)) {
LOG(debug) << "Sensitive Volume " << volume->GetName();
AddSensitiveVolume(volume);
}
Expand Down
4 changes: 2 additions & 2 deletions fairroot/base/sim/FairModule.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ void FairModule::ExpandNodeForGDML(TGeoNode* curNode)
AssignMediumAtImport(curVol);

// Check if the volume is sensitive
if ((this->InheritsFrom("FairDetector")) && IsSensitive(curVol->GetName())) {
if ((this->InheritsFrom("FairDetector")) && IsSensitive(curVol)) {
LOG(debug2) << "Sensitive Volume " << curVol->GetName();
AddSensitiveVolume(curVol);
}
Expand Down Expand Up @@ -564,7 +564,7 @@ void FairModule::ExpandNode(TGeoNode* fN)
LOG(debug2) << "Register Volume " << v->GetName();
v->RegisterYourself();
}
if ((this->InheritsFrom("FairDetector")) && IsSensitive(v->GetName())) {
if ((this->InheritsFrom("FairDetector")) && IsSensitive(v)) {
Copy link
Copy Markdown
Member

@ChristianTackeGSI ChristianTackeGSI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

this->InheritsFrom("FairDetector")

BTW: Can we please (in another PR!)

  • Move this out of the loop? It's run each time inside a loop!
  • Replace this by a (dynamic_cast<FairDetector>(this) != nullptr)?

LOG(debug2) << "Sensitive Volume " << v->GetName();
AddSensitiveVolume(v);
}
Expand Down
27 changes: 22 additions & 5 deletions fairroot/base/sim/FairModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "FairRuntimeDb.h" // for FairRuntimeDb

#include <TCollection.h> // for TRangeDynCast
#include <TGeoManager.h>
#include <TGeoMatrix.h>
#include <TGeoNode.h>
#include <TGeoVolume.h>
Expand Down Expand Up @@ -103,12 +104,28 @@ class FairModule : public TNamed
template<class T, class U>
void ConstructASCIIGeometry(TString containerName = "");

/**Set the sensitivity flag for volumes, called from ConstructASCIIRootGeometry(), and has to be implimented for
* detectors which use ConstructASCIIRootGeometry() to build the geometry */
virtual Bool_t IsSensitive(const std::string& name);
/**
* Check volume sensitivity by probing medium sensitivity flag.
*
* Function checks the sensitivity property of the medium. This is generic approach and should work for any volume.
* Override this _only_ in case of specific customisation.
*
* See Bool_t IsSensitive(const std::string& name) for deprecation notice.
*/
virtual bool IsSensitive(TGeoVolume* vol) { return (vol->GetMedium()->GetParam(0) == 1.0); }
Copy link
Copy Markdown
Member

@ChristianTackeGSI ChristianTackeGSI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF I got some of the notes correctly, then this is a standard attribute of the medium?

The 0 looks like a magic constant? Is there some const / define available for this?

Thinking about my backward compatible request, maybe the current default implementation should be return false;?

And in a later update we could use this one?

Yes, this means that everyone who wants to use this customization point would need to implement this code. But it would maybe help with PR acceptance?

Copy link
Copy Markdown
Author

@rlalik rlalik Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Geant 3 defienes ISVOL flag:
image

And we use TGeoMedium which defines it in similar way as integer:
https://root.cern.ch/doc/master/TGeoMedium_8cxx_source.html#l00064

There is no constant/enum defined, just 0 - not sensitive, > 0 - sensitive. TGeoMedium stores parameters as floating points. The proper check could be actually vol->GetMedium()->GetParam(0) > 0.0.

So, as I also wrote it before, this is bi-state, either volume is senstivie or non-sensitive. Thats way false as a return is also a valid result of the check. we would need tri-state return value to check whether the function was implemented good candidate would be std::optional with {} as return. But this also requires to change function signature.

Other idea - using exceptions. Throw in the base function and catch it, or the derived function returns valid value. Executing the string-based check will give some deprecation message, and perhaps full removal in the future (?).

Copy link
Copy Markdown
Member

@ChristianTackeGSI ChristianTackeGSI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant, whether there's a constant for the 0 in ->GetParam(0)? This really feels like a magic constant. I don't like those.

Copy link
Copy Markdown
Author

@rlalik rlalik Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, as you can find in line 84 of this code https://root.cern.ch/doc/master/TGeoMedium_8cxx_source.html#l00064, it is just magic number.


/**
* Set the sensitivity flag for volumes, called from ConstructASCIIRootGeometry(), and has to be implimented for
* detectors which use ConstructASCIIRootGeometry() to build the geometry
*/
[[deprecated("Remove override of this function. Use override of IsSensitive(TGeoVolume* vol) only if customisation "
"required. This function will be removed in the future releases.")]]
virtual Bool_t IsSensitive(const std::string& name) final;

/**The function below is depracated, please change to the new method above */
virtual Bool_t CheckIfSensitive(__attribute__((unused)) std::string name) __attribute__((
deprecated("The method CheckIfSensitive is deprecated. Implement IsSensitive in the detector classes.")))
[[deprecated("The method CheckIfSensitive is deprecated. Implement IsSensitive in the detector "
"classes. To be removed in the future.")]]
virtual Bool_t CheckIfSensitive(__attribute__((unused)) std::string name) final
{
return kFALSE;
}
Expand Down
Loading