feat(sim): Allow branch registration in `FairGenerator` #1575

karabowi · 2024-07-08T08:00:53Z

Introduced new function FairMCApplication::InitFinalizer() which initializes event generator, tasks, and triggers FairRootManager::WriteFolder() function.
This new function is called from the latter of
InitGeometry() and AddIons().

Previously the funcionality of InitFinalizer was split between InitGeometry() and AddIons(), which caused problems because
TGeant3 calls AddIons() first, InitGeometry() second, TGeant4 calls InitGeometry() first, AddIons() second.

Current implementation assures that initialization of event generators and tasks come before WriteFolder.

Fixes issues #1183 and #1567.

Checklist:

Followed the Contributing Guidelines

coderabbitai · 2024-07-08T08:01:00Z

Walkthrough

The changes enhance the initialization workflow of the FairMCApplication class within Fairroot by introducing the InitFinalizer() method, which consolidates event generator and task initialization. The state management has been refined with the addition of new states and modifications to existing methods like AddIons() and InitGeometry(). Additionally, new methods for folder management have been added to the FairRootManager class to facilitate dynamic folder removal from the ROOT directory.

Changes

File Path	Change Summary
`fairroot/base/sim/FairMCApplication.cxx`	Restructured initialization; added `InitFinalizer()`, updated state transitions in `InitMC`, `RunMC`, and `InitGeometry`, modified `AddIons()`
`fairroot/base/sim/FairMCApplication.h`	Updated state enum, added `InitFinalizer()` method, streamlined comments, introduced `fIonsAreAdded` variable
`fairroot/base/steer/FairRootManager.cxx`	Added `RemoveFolder()` and `RemoveOutputFolderForMtMode()` methods for dynamic folder removal
`fairroot/base/steer/FairRootManager.h`	Added declarations for `RemoveFolder()` and `RemoveOutputFolderForMtMode()` methods; reorganized include statements
`fairroot/base/steer/FairRunSim.h`	Added documentation comment for `SetMCConfig()` method to clarify its responsibilities
`examples/simulation/rutherford/macros/CMakeLists.txt`	Modified command for running `run_rutherford.sh` to include an additional boolean argument

Possibly related PRs

refactor(Base): Move GetRootManager to FairModule #1574: The changes in FairMCApplication.cxx within this PR involve modifications to the FairMCApplication class, which is directly related to the changes made in the main PR regarding state management and method alterations in the same file.
refactor: Drop unneeded FRM references and empty methods #1577: This PR involves removing unneeded references to FairRootManager, which is relevant as the main PR also modifies the FairMCApplication class and its interaction with the FairRootManager, indicating a connection in the context of managing application states and resources.

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d84e17f and 63b6d7f.

Files selected for processing (7)

examples/simulation/rutherford/macros/CMakeLists.txt (1 hunks)
fairroot/base/sim/FairMCApplication.cxx (7 hunks)
fairroot/base/sim/FairMCApplication.h (6 hunks)
fairroot/base/sink/FairRootFileSink.cxx (0 hunks)
fairroot/base/steer/FairRootManager.cxx (3 hunks)
fairroot/base/steer/FairRootManager.h (2 hunks)
fairroot/base/steer/FairRunSim.h (1 hunks)

Files not reviewed due to no reviewable changes (1)

fairroot/base/sink/FairRootFileSink.cxx

Files skipped from review as they are similar to previous changes (6)

examples/simulation/rutherford/macros/CMakeLists.txt
fairroot/base/sim/FairMCApplication.cxx
fairroot/base/sim/FairMCApplication.h
fairroot/base/steer/FairRootManager.cxx
fairroot/base/steer/FairRootManager.h
fairroot/base/steer/FairRunSim.h

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-- I pushed a fix in commit <commit_id>, please review it.
-- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
-- @coderabbitai generate unit testing code for this file.
-- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
-- @coderabbitai generate interesting stats about this repository and render them as a table.
-- @coderabbitai read src/utils.ts and generate unit testing code.
-- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
-- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

dennisklein · 2024-07-08T08:36:41Z

Could you please add some documentation of the FairMCApplication state machine? At least on the initialization part? After the FairMCApplication is constructed, e.g. which of the various Init* member functions are supposed to be called in which order to complete a successful initialization? Just for internal code documentation. Thx!

fairroot/base/sim/FairMCApplication.cxx

karabowi · 2024-07-09T13:06:57Z

Could you please add some documentation of the FairMCApplication state machine? At least on the initialization part? After the FairMCApplication is constructed, e.g. which of the various Init* member functions are supposed to be called in which order to complete a successful initialization? Just for internal code documentation. Thx!

It is too hot to work;).

I wish it was that easy. Lets consider three FairMCApplication's functions: ConstructGeometry, InitGeometry and AddIons. They are all called internally from TGeant3 or TGeant4.

FairRunSim runs the simulation initialization in SetMCConfig() function, which:

runs simulation config (mostly the infamous macros). TGeant4 calls ConstructGeometry in the constructor.
runs FairMCApplication->InitMC(), which:
- starts Stack
- runs fMC->Init(), TGeant4 calls InitGeometry here, while TGeant3 calls AddIons, ConstructGeometry and InitGeometry here.
- runs fMC->BuildPhysics(), TGeant4 calls AddIons here
runs postinit config.

What do you think about such a state order: kPreInit -> kConstructGeometry -> kInit -> kInitGeometry -> kPostInit -> kRun?
I will implement that and lets see.

dennisklein · 2024-07-09T13:37:29Z

What do you think about such a state order: kPreInit -> kConstructGeometry -> kInit -> kInitGeometry -> kPostInit -> kRun?
I will implement that and lets see.

What do you mean by that? Implement what?

ChristianTackeGSI · 2024-07-09T13:45:03Z

@karabowi reopned this

If you wanted to rerun the CI due to the errors:

Please rebase to latest dev. It has a bunch of fixes that affect the CI.
You should be able to pree some re-run button in jenkins.

ChristianTackeGSI · 2024-07-09T13:46:49Z

FairRunSim runs the simulation initialization in SetMCConfig() function, which:
…

Can we document exactly this somewhere, please?

Maybe on FairRunSim and/or FairMCApplication?

dennisklein · 2024-07-09T14:37:50Z

What do you think about such a state order: kPreInit -> kConstructGeometry -> kInit -> kInitGeometry -> kPostInit -> kRun?
I will implement that and lets see.

What do you mean by that? Implement what?

Now I see the confusion, sry, I was not referring to the fState member variable. I was just using the term "state machine" loosely to refer to the multi-phase initialization design of the FairMCApplication class. What I am looking for is some documentation on each of then Init*-type functions. Basically, what are their pre- and post-conditions.

I now also see that this design is partly coming from VMC and FairRoot attached all kinds of additional state to this class. I give up, nevermind my request for documentation.

karabowi · 2024-07-10T10:26:51Z

FairRunSim runs the simulation initialization in SetMCConfig() function, which:
…

Can we document exactly this somewhere, please?

Maybe on FairRunSim and/or FairMCApplication?

Done.

fairroot/base/sim/FairMCApplication.h

fairroot/base/sim/FairMCApplication.cxx

ChristianTackeGSI

Generally, I like the way this is going!

So maybe we should merge it as a "step in the right direction"?

That said, I don't know, whether some things should be squashed?

fairroot/base/sim/FairMCApplication.h

ChristianTackeGSI

Okay, you're right! There's no need for squashing! Things look good from that point of view.

While looking again at the individual commits, I noticed another small thing.

fairroot/base/sim/FairMCApplication.cxx

ChristianTackeGSI

Please improve naming. :-)

fairroot/base/steer/FairRootManager.h

fairroot/base/steer/FairRootManager.cxx

ChristianTackeGSI · 2024-09-13T16:51:14Z

Hi @karabowi,

FairRoot/fairroot/base/sink/FairRootFileSink.cxx

Lines 249 to 250 in 6decd1e

    
           // Delete the folder to make place in the gROOT for fOutFolder created by the Geant4MT threads 
        
           gROOT->GetRootFolder()->Remove(fOutFolder);

do you want to remove this? Or keep it for backward compatibility?

Introduced new function `FairMCApplication::InitFinalizer()` which initializes event generator, tasks, and triggers `FairRootManager::WriteFolder()` function. This new function is called from `InitMC()` and from `InitOnWorker()` for TGeant4 MT mode. Previously the funcionality of `InitFinalizer` was split between `InitGeometry()` and `AddIons()`, which caused problems because TGeant3 calls `AddIons()` first, `InitGeometry()` second, TGeant4 calls `InitGeometry()` first, `AddIons()` second. Current implementation assures that initialization of event generators and tasks come before `WriteFolder`. Fixes issues FairRootGroup#1183 and FairRootGroup#1567.

Rutherford example does not need to run in MT mode, since we have dedicated MT test anyways. Changed the test not to run in MT mode.

Comments explain `SetMCConfig()` functionality.

Added few states as well as few checks. Previous state order: `kUnknownState` -> `kConstructGeometry` -> `kUnknownState` -> `kInitGeometry` -> `kUnknownState` Currently: `kPreInit` -> `kConstructGeometry` -> `kInit` -> `kInitGeometry` -> `kInit` -> `kPostInit` -> `kRun`.

The function `WriteFolder` (since the introduction of the `MTMode`) was misused to remove the `TFolder` from `gROOT` after it was used to create the output tree. Purely for that reason `WriteFolder` was called in the `InitGeometry()`. This folder is no longer removed in the sink's `WriteFolder()`. Introduced a new `FairRootManager::RemoveOutputFolderForMtMode()` function which removes the `TFolder` from `gROOT`, which is needed in the MT mode in TGeant4 transport. Used this function in `FairMCApplicaiton::InitGeometry()` and in `FairMCApplication::InitFinalizer()`.

karabowi · 2024-09-16T07:12:46Z

Hi @karabowi,

FairRoot/fairroot/base/sink/FairRootFileSink.cxx

Lines 249 to 250 in 6decd1e

// Delete the folder to make place in the gROOT for fOutFolder created by the Geant4MT threads

gROOT->GetRootFolder()->Remove(fOutFolder);

do you want to remove this? Or keep it for backward compatibility?

I have removed this line, or rather moved it to FairMCApplication::InitFinalizer().

ChristianTackeGSI reviewed Jul 8, 2024

View reviewed changes

fairroot/base/sim/FairMCApplication.cxx Outdated Show resolved Hide resolved

karabowi closed this Jul 9, 2024

karabowi reopened this Jul 9, 2024

karabowi force-pushed the feat_gen_branch branch 2 times, most recently from 24e4c0b to 3174050 Compare July 10, 2024 08:57

karabowi mentioned this pull request Jul 11, 2024

Cannot register any branch in the Init() of FairGenerator #1567

Closed

ChristianTackeGSI reviewed Jul 17, 2024

View reviewed changes

fairroot/base/sim/FairMCApplication.h Outdated Show resolved Hide resolved

fairroot/base/sim/FairMCApplication.h Outdated Show resolved Hide resolved

fairroot/base/sim/FairMCApplication.cxx Outdated Show resolved Hide resolved

karabowi force-pushed the feat_gen_branch branch 2 times, most recently from 07d4b38 to 8023a59 Compare July 23, 2024 14:46

ChristianTackeGSI reviewed Aug 24, 2024

View reviewed changes

fairroot/base/sim/FairMCApplication.h Outdated Show resolved Hide resolved

karabowi force-pushed the feat_gen_branch branch from 8023a59 to b4a2bfc Compare August 28, 2024 10:19

ChristianTackeGSI reviewed Aug 29, 2024

View reviewed changes

fairroot/base/sim/FairMCApplication.cxx Outdated Show resolved Hide resolved

karabowi force-pushed the feat_gen_branch branch from b4a2bfc to 3fb53f0 Compare September 10, 2024 13:12

ChristianTackeGSI reviewed Sep 10, 2024

View reviewed changes

fairroot/base/steer/FairRootManager.h Outdated Show resolved Hide resolved

fairroot/base/steer/FairRootManager.cxx Outdated Show resolved Hide resolved

karabowi force-pushed the feat_gen_branch branch 3 times, most recently from 134646f to d84e17f Compare September 13, 2024 16:13

ChristianTackeGSI previously approved these changes Sep 13, 2024

View reviewed changes

karabowi added 3 commits September 16, 2024 08:57

chore(examples/rutherford): Switch MT mode off in the test

4ab59c2

Rutherford example does not need to run in MT mode, since we have dedicated MT test anyways. Changed the test not to run in MT mode.

docs(sim): Add comment to FairRunSim::SetMCConfig()

9235df8

Comments explain `SetMCConfig()` functionality.

karabowi added 2 commits September 16, 2024 08:57

karabowi dismissed ChristianTackeGSI’s stale review via 63b6d7f September 16, 2024 06:57

karabowi force-pushed the feat_gen_branch branch from d84e17f to 63b6d7f Compare September 16, 2024 06:57

ChristianTackeGSI approved these changes Sep 16, 2024

View reviewed changes

ChristianTackeGSI merged commit f5fc5c4 into FairRootGroup:dev Sep 16, 2024
30 checks passed

This was referenced Sep 16, 2024

refactor(Base): Introduce FairRootManager::CreateOutputFolder #1591

Merged

Introduce FairExampleRunSim #1431

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sim): Allow branch registration in `FairGenerator` #1575

feat(sim): Allow branch registration in `FairGenerator` #1575

karabowi commented Jul 8, 2024

coderabbitai bot commented Jul 8, 2024 •

edited

Loading

Walkthrough

Changes

Possibly related PRs

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

dennisklein commented Jul 8, 2024

karabowi commented Jul 9, 2024 •

edited

Loading

dennisklein commented Jul 9, 2024

ChristianTackeGSI commented Jul 9, 2024

ChristianTackeGSI commented Jul 9, 2024

dennisklein commented Jul 9, 2024 •

edited

Loading

karabowi commented Jul 10, 2024

ChristianTackeGSI left a comment

ChristianTackeGSI left a comment

ChristianTackeGSI left a comment

ChristianTackeGSI commented Sep 13, 2024

karabowi commented Sep 16, 2024

feat(sim): Allow branch registration in FairGenerator #1575

feat(sim): Allow branch registration in FairGenerator #1575

Conversation

karabowi commented Jul 8, 2024

coderabbitai bot commented Jul 8, 2024 • edited Loading

Walkthrough

Changes

Possibly related PRs

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

dennisklein commented Jul 8, 2024

karabowi commented Jul 9, 2024 • edited Loading

dennisklein commented Jul 9, 2024

ChristianTackeGSI commented Jul 9, 2024

ChristianTackeGSI commented Jul 9, 2024

dennisklein commented Jul 9, 2024 • edited Loading

karabowi commented Jul 10, 2024

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

ChristianTackeGSI commented Sep 13, 2024

karabowi commented Sep 16, 2024

feat(sim): Allow branch registration in `FairGenerator` #1575

feat(sim): Allow branch registration in `FairGenerator` #1575

coderabbitai bot commented Jul 8, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

karabowi commented Jul 9, 2024 •

edited

Loading

dennisklein commented Jul 9, 2024 •

edited

Loading