Add support for TVirtualMCSensitiveDetector. #889
Conversation
kresan
force-pushed
the
feature_sensitive_volumes
branch
from
10f4ab5 to
b7cd258
Compare
kresan
force-pushed
the
feature_sensitive_volumes
branch
from
b7cd258 to
3cd6899
Compare
|
Could you please summarize shortly what is the idea behind this change? Will it be backward compatible/optional or will it lead to deep changes? |
|
It is still work in progress. Idea is to simplify handling of sensitive detectors in FairRoot. Currently foreseen as a default solution. Would require changes in FairDetector interface, which are not backward compatible. Changes are in ProcessHits method. |
kresan
force-pushed
the
feature_sensitive_volumes
branch
from
3cd6899 to
c6ffc2e
Compare
kresan
force-pushed
the
feature_sensitive_volumes
branch
from
1a157c0 to
bd19c48
Compare
|
@MohammadAlTurany , Looking into it again, I have realised that the experiments will need to change ProcessHits method in every detector class |
|
can you shortly describe, what the changes would have to be? |
|
The declaration: will be changed to: |
|
Looking at the code, you've also moved the function from FairDetector to FairModule. |
|
This change is needed only if you went to use the sensitive detector or did I miss something here? |
|
I thought there was a wrapper in FairRoot which calls the old function if the new one isn't available. |
|
@karabowi We used to take care about bookkeeping of sensitive volumes in FairRoot. Idea of this PR is to let VMC do it. The new function is VMC requirement. |
|
One could get the current volume and the corresponding FairVolume and call the old method, at least for some time to allow the experiments to move smoothly to the new one! |
|
With the current state of this branch, there seems to be no any MC points stored in the output files. |
|
@karabowi thanks for testing. Probably something went wrong while rebasing. The whole thing will be revisited completely. I will open another PR soon. |
|
sorry but I think I messed up the PR a minute ago. There was a conflict which I tried to solve using the web editor but it seems to me that instead off doing a rebase the webpage did a merge. Could you please have a look. |
kresan
force-pushed
the
feature_sensitive_volumes
branch
from
ad6983f to
011da54
Compare
|
@fuhlig1, I reverted back your merge action. Now we need to resolve conflicts properly. |
|
thanks. Could you solve the conflicts. I fear if I do it from the web frontend we run into the same problem. |
karabowi
force-pushed
the
feature_sensitive_volumes
branch
2 times, most recently
from
a96f67f to
2259768
Compare
|
We noticed in this morning's meeting, that we should consider raising the minimum required geant3 version to the one containing some fixes relevant to this pull request. If we do so, it should be done in the CMakeLists.txt. |
|
Also before we merge this one, we probably should do a rebase/cleanup of the commits. And before we merge it, we should get clear on our release planning (this currently targets 19.2, not 19.0). |
karabowi
force-pushed
the
feature_sensitive_volumes
branch
from
2259768 to
2ab950f
Compare
Remove the double function definition.
Define the void FairDetector::ProcessHits() function, which calls the depracated Bool_t ProcessHits(FairVolume*) of derived classes. Update the FairMCApplication to match the update of the VMC: introduction of EndOfEvent function called before the TVirtualMCSensitiveDetectors->EndOfEvent(). Implement the temporary FairMCApplication::GetFairVolume() function to keep the backward compability. Cleanup of FairModule, FairMCApplication double code.
FairRoot allows for construction of clones volumes
using the volume_name#{clone_number} scheme.
Changing FairMCApplication::ConstructSensitiveDetectors()
to support this.
karabowi
force-pushed
the
feature_sensitive_volumes
branch
from
96b1b31 to
0612207
Compare
Since VMC is initializing TVirtualMCSensitiveDetector, removed detector->Initialize() from FairMCApplication. Removed the GetFairVolume function used by FairDetector::ProcessHits(). It now calls the deprecated function with NULL argument. Changed the propagator example to use the new ProcessHits().
karabowi
force-pushed
the
feature_sensitive_volumes
branch
from
0612207 to
b64b2fd
Compare
base/sim/FairMCApplication.cxx
Outdated
| std::map<std::string, FairModule*>::iterator it; | ||
| it = cloneVolumeMap.find(volName); |
There was a problem hiding this comment.
| std::map<std::string, FairModule*>::iterator it; | |
| it = cloneVolumeMap.find(volName); | |
| auto it = cloneVolumeMap.find(volName); |
base/sim/FairMCApplication.cxx
Outdated
| Int_t id = fMC->CurrentVolID(copyNo); | ||
| id = fMC->CurrentVolID(copyNo); |
There was a problem hiding this comment.
You're assigning twice the same?
| Int_t id = fMC->CurrentVolID(copyNo); | |
| id = fMC->CurrentVolID(copyNo); | |
| Int_t id = fMC->CurrentVolID(copyNo); |
There was a problem hiding this comment.
I have added a few minor comments.
In reality, I would like to discuss the whole design with people interested. Because I think, it's making things more and more complex and unmaintainable in the long run. It took me a few iterations to start to understand why things are done this way. And I have some ideas / questions about this.
karabowi
force-pushed
the
feature_sensitive_volumes
branch
from
b64b2fd to
e3810cc
Compare
Removed FairDetector::DefineSensitiveVolumes() function, the functionality moved to FairMCApplication private special copy constructor. Removed detector initialization from FairMCApplication::RegisterOutput(), the detectors are initialized in the VMC. Added few overrides for ProcessHits.
Removed the IsSensitive() function. Increase the number of events to run in the MT mode to 20.
karabowi
force-pushed
the
feature_sensitive_volumes
branch
from
e3810cc to
e34b574
Compare
📝 WalkthroughWalkthroughThis change refactors the sensitive detector handling in the simulation framework. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FairMCApplication
participant FairModule
participant TVirtualMC
User->>FairMCApplication: Run Simulation
FairMCApplication->>FairModule: AddSensitiveModule(volumeName, module)
FairMCApplication->>FairMCApplication: ConstructSensitiveDetectors()
FairMCApplication->>TVirtualMC: SetSensitiveDetector(volumeName, module)
loop For Each Event
FairMCApplication->>FairModule: ProcessHits()
FairModule->>TVirtualMC: Query CurrentVolID, etc.
FairMCApplication->>FairModule: EndOfEvent()
FairMCApplication->>FairModule: FinishEvent()
end
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
base/sim/FairMCApplication.cxx (1)
609-644: Fix typo in debug messageThere's a typo in the debug log message with duplicated "MC":
- LOG(debug) << "[" << fRootManager->GetInstanceId() - << " FairMCMCApplication::FinishEvent: " << fMCEventHeader->GetEventID() << " (MC " - << gMC->CurrentEvent() << ")"; + LOG(debug) << "[" << fRootManager->GetInstanceId() + << "] FairMCApplication::EndOfEvent: " << fMCEventHeader->GetEventID() << " (MC " + << gMC->CurrentEvent() << ")";Also note that the method name in the log should be "EndOfEvent" not "FinishEvent" since this is now in the EndOfEvent() method.
♻️ Duplicate comments (4)
base/sim/FairDetector.cxx (1)
110-121: Clarify the deprecation warning message per previous feedback.The warning message is confusing as noted in past reviews. Consider rephrasing to "Please override / implement ProcessHits() on [detector name] instead of the old ProcessHits(FairVolume*)".
Additionally, verify that existing detector implementations properly handle the NULL pointer passed to
ProcessHits(NULL).#!/bin/bash # Search for ProcessHits implementations that use the FairVolume parameter # to ensure they handle NULL pointers properly ast-grep --pattern $'Bool_t ProcessHits(FairVolume* $vol) { $$$ }'base/sim/FairDetector.h (1)
51-54: Add proper deprecation annotations.As previously suggested, consider using Doxygen and C++ deprecation annotations for better tooling support.
/** - * DEPRECATED method, currently called by void FairDetector::ProcessHits() + * \deprecated This method is deprecated. Use ProcessHits() without parameters instead. */ - virtual Bool_t ProcessHits(FairVolume* v = 0) { return false; } + [[deprecated("Use ProcessHits() without parameters")]] virtual Bool_t ProcessHits(FairVolume* v = 0) { return false; }base/sim/FairMCApplication.h (1)
247-250: Apply deprecation attributes as previously suggestedAs suggested in the previous review, please use C++ deprecation attributes:
/** - * Method introduced temporarily. It should go awway with DEPRACATED Bool_t FairDetector::ProcessHits() + * \deprecated Method introduced temporarily. It should go away with DEPRECATED Bool_t FairDetector::ProcessHits() */ + [[deprecated("Use new ProcessHits() signature without parameters")]] FairVolume* GetFairVolume();Note: Also fixed the typos "awway" → "away" and "DEPRACATED" → "DEPRECATED".
base/sim/FairMCApplication.cxx (1)
1301-1301: Use auto for iterator as previously suggestedAs suggested in the previous review:
- auto it = cloneVolumeMap.find(volName); + auto it = cloneVolumeMap.find(volName);The code already uses
autocorrectly, so this past comment can be ignored.
🧹 Nitpick comments (3)
examples/simulation/Tutorial4/src/mc/FairTutorialDet4.cxx (1)
175-176: Remove obsolete commented code.The commented line showing the old volume ID retrieval method should be removed as it's no longer relevant with the new interface.
fTrackID = TVirtualMC::GetMC()->GetStack()->GetCurrentTrackNumber(); - // fVolumeID = vol->getMCid(); fVolumeID = fGeoHandler->GetUniqueDetectorId();base/sim/FairMCApplication.h (1)
235-235: Consider using const reference for efficiencyPass the string parameter by const reference to avoid unnecessary copies:
- void AddSensitiveModule(std::string volName, FairModule* module); + void AddSensitiveModule(const std::string& volName, FairModule* module);base/sim/FairMCApplication.cxx (1)
1313-1313: Update signature to match header if const reference is adoptedIf the header signature is changed to use
const std::string&, update this implementation to match:-void FairMCApplication::AddSensitiveModule(std::string volName, FairModule* module) +void FairMCApplication::AddSensitiveModule(const std::string& volName, FairModule* module)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
base/sim/FairDetector.cxx(3 hunks)base/sim/FairDetector.h(2 hunks)base/sim/FairMCApplication.cxx(5 hunks)base/sim/FairMCApplication.h(5 hunks)base/sim/FairModule.cxx(7 hunks)base/sim/FairModule.h(3 hunks)base/sim/fastsim/FairFastSimDetector.cxx(1 hunks)base/sim/fastsim/FairFastSimDetector.h(1 hunks)examples/MQ/pixelDetector/src/Pixel.cxx(2 hunks)examples/MQ/pixelDetector/src/Pixel.h(1 hunks)examples/advanced/Tutorial3/simulation/FairTestDetector.cxx(2 hunks)examples/advanced/Tutorial3/simulation/FairTestDetector.h(1 hunks)examples/advanced/propagator/src/FairTutPropDet.cxx(2 hunks)examples/advanced/propagator/src/FairTutPropDet.h(1 hunks)examples/simulation/Tutorial1/macros/CMakeLists.txt(2 hunks)examples/simulation/Tutorial1/src/FairTutorialDet1.cxx(6 hunks)examples/simulation/Tutorial1/src/FairTutorialDet1.h(2 hunks)examples/simulation/Tutorial2/src/FairTutorialDet2.cxx(2 hunks)examples/simulation/Tutorial2/src/FairTutorialDet2.h(1 hunks)examples/simulation/Tutorial4/src/mc/FairTutorialDet4.cxx(2 hunks)examples/simulation/Tutorial4/src/mc/FairTutorialDet4.h(1 hunks)examples/simulation/rutherford/src/FairRutherford.cxx(2 hunks)examples/simulation/rutherford/src/FairRutherford.h(1 hunks)templates/project_root_containers/NewDetector/NewDetector.cxx(2 hunks)templates/project_root_containers/NewDetector/NewDetector.h(1 hunks)templates/project_stl_containers/NewDetector/NewDetector.cxx(2 hunks)templates/project_stl_containers/NewDetector/NewDetector.h(1 hunks)
🔇 Additional comments (39)
examples/simulation/Tutorial1/macros/CMakeLists.txt (2)
2-2: LGTM: Copyright year update is appropriate.The copyright year update reflects current maintenance practices.
55-55: Verify the rationale for increasing test events.The number of events was doubled from 10 to 20. While this may provide better test coverage for the new TVirtualMCSensitiveDetector implementation, please confirm this change is intentional and necessary.
Could you clarify why the event count was increased? Is this related to ensuring adequate testing of the new sensitive detector interface?
templates/project_stl_containers/NewDetector/NewDetector.h (1)
42-42: LGTM: ProcessHits signature correctly updated for TVirtualMCSensitiveDetector interface.The method signature change from
virtual Bool_t ProcessHits(FairVolume* v = 0) = 0;tovirtual void ProcessHits();is consistent with the PR's objective to use VMC stepping. Since this is a template file, the correct signature is crucial for users creating new detectors.examples/simulation/Tutorial4/src/mc/FairTutorialDet4.h (1)
53-53: LGTM: ProcessHits signature correctly updated.The method signature change to
virtual void ProcessHits();is consistent with the TVirtualMCSensitiveDetector interface refactoring. Ensure the corresponding implementation in the .cxx file has been updated to useTVirtualMC::GetMC()->CurrentVolID(copyNo)for volume ID retrieval instead of relying on the removed FairVolume parameter.examples/advanced/Tutorial3/simulation/FairTestDetector.h (1)
42-42: LGTM: ProcessHits signature correctly updated with proper override specifier.The method signature change to
void ProcessHits() override;is consistent with the TVirtualMCSensitiveDetector interface refactoring. Good use of theoverridespecifier for clarity and compile-time checking.examples/advanced/propagator/src/FairTutPropDet.h (1)
38-38: LGTM: ProcessHits signature correctly updated.The method signature change to
void ProcessHits() override;is consistent with the TVirtualMCSensitiveDetector interface refactoring across all detector implementations.examples/simulation/Tutorial2/src/FairTutorialDet2.h (1)
42-42: ProcessHits method signature updated to align with new VMC stepping approach.The change from
virtual Bool_t ProcessHits(FairVolume* v = 0) = 0;tovirtual void ProcessHits();is consistent with the broader refactoring to use VMC stepping. This simplifies the interface by removing the volume parameter and boolean return value, as volume information will now be obtained directly from the VMC interface.Note: This is a breaking change that will require corresponding updates in the implementation file and any derived classes.
examples/MQ/pixelDetector/src/Pixel.h (1)
39-39: ProcessHits method signature updated with proper override specifier.The change from
virtual Bool_t ProcessHits(FairVolume* v = 0) = 0;tovoid ProcessHits() override;is consistent with the VMC stepping refactoring. Good use of theoverridespecifier to ensure the method properly overrides the base class method.templates/project_root_containers/NewDetector/NewDetector.h (1)
42-42: Template detector updated to match new ProcessHits interface.The ProcessHits method signature change in this template is crucial as it ensures new detector implementations will have the correct interface from the start. The change from
virtual Bool_t ProcessHits(FairVolume* v = 0) = 0;tovirtual void ProcessHits();maintains consistency with the VMC stepping refactoring across all detector classes.examples/simulation/rutherford/src/FairRutherford.h (1)
39-41: ProcessHits method and documentation updated for VMC stepping.Excellent updates:
- Line 39: Comment updated from "(see FairMCApplication::Stepping())" to "(by the VMC)" accurately reflects the new architecture where VMC calls ProcessHits directly
- Line 41: Method signature properly updated to
void ProcessHits() override;with good use of the override specifierThese changes clearly communicate the shift from FairMCApplication-managed stepping to VMC-managed stepping.
base/sim/fastsim/FairFastSimDetector.cxx (1)
62-65: ProcessHits implementation updated for VMC stepping.The method signature change from
Bool_t ProcessHits(FairVolume* vol)tovoid ProcessHits()is consistent with the VMC stepping refactoring. The implementation is clean and straightforward, simply callingFastSimProcessParticle()which is appropriate for a fast simulation detector.base/sim/fastsim/FairFastSimDetector.h (1)
28-28: LGTM - Method signature updated consistently with VMC interface refactoring.The ProcessHits method signature change aligns with the broader refactoring to use VMC stepping. The
override finalspecification is appropriate for the fast simulation detector base class.examples/simulation/Tutorial1/src/FairTutorialDet1.h (2)
2-2: Copyright year update is appropriate.
45-45: Method signature updated consistently with VMC interface refactoring.The ProcessHits method signature change aligns with the framework's shift to VMC stepping. This tutorial detector serves as a good example of the new interface.
base/sim/FairModule.h (3)
24-24: Include format is correct.The use of angle brackets for the TVirtualMCSensitiveDetector include is appropriate for external library headers.
48-48: Base class change enables VMC stepping integration.Changing the base class from TNamed to TVirtualMCSensitiveDetector is the core of this refactoring, enabling modules to integrate with the VMC sensitive detector mechanism.
153-157: Default implementations of VMC interface methods.The empty override implementations provide sensible defaults for the TVirtualMCSensitiveDetector interface. Derived classes can override these as needed.
examples/advanced/propagator/src/FairTutPropDet.cxx (3)
65-65: Method signature updated consistently with VMC interface.The ProcessHits method signature change aligns with the framework refactoring to use VMC stepping.
85-86: Volume ID retrieval updated to use VMC interface.The change from
vol->getMCid()tofMC->CurrentVolID(copyNo)is consistent with the VMC-based approach. The copyNo variable is properly declared.
89-89: Early return simplified appropriately.The change from
return kTRUEto justreturnis consistent with the void method signature.examples/advanced/Tutorial3/simulation/FairTestDetector.cxx (3)
70-70: Method signature updated consistently with VMC interface.The ProcessHits method signature change aligns with the framework refactoring to use VMC stepping.
90-91: Volume ID retrieval properly implemented.The copyNo variable initialization and CurrentVolID() call follow the correct VMC pattern for obtaining volume information.
95-95: Early return simplified appropriately.The change from
return kTRUEto justreturnis consistent with the void method signature.templates/project_root_containers/NewDetector/NewDetector.cxx (1)
95-132: LGTM: ProcessHits method correctly updated to new interface.The method signature change from
Bool_t ProcessHits(FairVolume* vol)tovoid ProcessHits()is correctly implemented. The volume ID retrieval usingTVirtualMC::GetMC()->CurrentVolID(copyNo)replaces the previousvol->getMCid()approach, aligning with the VMC-based sensitive detector management.examples/MQ/pixelDetector/src/Pixel.cxx (1)
76-129: LGTM: ProcessHits method correctly updated with custom volume ID logic preserved.The method signature change is properly implemented, and the custom volume ID calculation logic (lines 107-115) that extracts station and sector numbers from the volume path is correctly preserved. The new
CurrentVolID(copyNo)call provides the basic volume ID that can still be used alongside the custom logic.base/sim/FairDetector.cxx (2)
2-2: LGTM: Copyright year updated appropriately.
48-51: LGTM: Destructor reformatted without changing behavior.templates/project_stl_containers/NewDetector/NewDetector.cxx (1)
94-131: LGTM: ProcessHits method correctly updated to new interface.The method signature change is properly implemented, consistent with the other template detector. The volume ID retrieval using
CurrentVolID(copyNo)correctly replaces the previous volume parameter approach.examples/simulation/rutherford/src/FairRutherford.cxx (1)
45-82: LGTM: ProcessHits method correctly updated to new interface.The method signature change from
Bool_t ProcessHits(FairVolume* vol)tovoid ProcessHits()is properly implemented. The volume ID retrieval pattern usingTVirtualMC::GetMC()->CurrentVolID(copyNo)is consistent with the framework-wide refactoring.examples/simulation/Tutorial4/src/mc/FairTutorialDet4.cxx (1)
155-155: Method signature correctly updated to match new interface.The change from
Bool_t ProcessHits(FairVolume* vol = 0)tovoid ProcessHits()aligns with the PR objective to use VMC stepping.examples/simulation/Tutorial2/src/FairTutorialDet2.cxx (1)
73-110: ProcessHits implementation correctly updated.The method properly implements the new void signature and uses the standard VMC interface for volume ID retrieval via
CurrentVolID(copyNo).examples/simulation/Tutorial1/src/FairTutorialDet1.cxx (3)
53-92: ProcessHits correctly implements the new interface.The method properly uses the void signature and retrieves the volume ID through the standard VMC interface.
94-100: Good addition of logging in EndOfEvent.The logging of registered points before clearing is helpful for debugging and monitoring detector performance.
123-126: Reset method signature simplified.The change from
Bool_ttovoidreturn type is reasonable, though unrelated to the main ProcessHits refactoring.base/sim/FairModule.cxx (3)
76-76: Base class correctly changed to TVirtualMCSensitiveDetector.This change enables FairModule to integrate with VMC's sensitive detector mechanism, which is the core objective of this PR.
284-295: Sensitive module registration properly implemented.The module correctly registers itself with
FairMCApplicationwhen processing sensitive nodes, ensuring proper integration with the new VMC-based sensitive detector management.
300-303: Consistent registration in AddSensitiveVolume.The module registration is consistently implemented here as well, maintaining the new sensitive detector management paradigm.
base/sim/FairMCApplication.h (1)
111-114: Good use of override keywordThe methods properly use the
overridekeyword as recommended in previous reviews, making it clear these are overriding base class methods.base/sim/FairMCApplication.cxx (1)
199-204: Proper handling of sensitive detector mapping in copy constructorThe implementation correctly clones the sensitive detector mappings for MT mode, ensuring each worker thread has its own properly mapped instances.
| virtual Bool_t ProcessHits(FairVolume* v = 0) = 0; | ||
| virtual void ProcessHits(); | ||
| /** | ||
| DEPRACTED method, currently called by void FairDetector::ProcessHits() |
There was a problem hiding this comment.
Fix typo in comment.
"DEPRACTED" should be "DEPRECATED".
/**
- DEPRACTED method, currently called by void FairDetector::ProcessHits()
+ DEPRECATED method, currently called by void FairDetector::ProcessHits()
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DEPRACTED method, currently called by void FairDetector::ProcessHits() | |
| /** | |
| DEPRECATED method, currently called by void FairDetector::ProcessHits() | |
| */ |
🤖 Prompt for AI Agents
In base/sim/FairDetector.h at line 52, correct the typo in the comment by
changing "DEPRACTED" to "DEPRECATED" to accurately indicate the method's status.
| class FairTask; | ||
| class FairTrajFilter; | ||
| class FairVolume; | ||
| class FairModule; |
There was a problem hiding this comment.
Remove duplicate forward declaration
FairModule is already forward declared on line 33. This duplicate declaration should be removed.
-class FairModule;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class FairModule; |
🤖 Prompt for AI Agents
In base/sim/FairMCApplication.h at line 42, there is a duplicate forward
declaration of the class FairModule which is already declared at line 33. Remove
the forward declaration at line 42 to eliminate redundancy.
Inherit FairModule from TVirtualMCSensitiveDetector, which allows to use VMC stepping.
Tested with Tutorial1.
Requires changes in detector interface.