search

FairRootGroup / FairRoot

C++ simulation, reconstruction and analysis framework for particle physics experiments
http://fairroot.gsi.de
Other
57 stars 96 forks source link

refactor(Base): Introduce FairGeoLoader::LoadAndCreate #1544

Closed ChristianTackeGSI closed 1 month ago

ChristianTackeGSI commented 3 months ago

... and use it around

@fuhlig1: you looked a lot at GeoSets lately. Can you have a look at this? Especially it's only used in ASCII related contexts, so maybe it should be called PrepareASCIIGeoSet?

Also I am not really sure on how to write tests for this.

Checklist:

coderabbitai[bot] commented 3 months ago

Walkthrough

The update focuses on simplifying and refactoring how geometry is handled in the FairRoot framework. It removes outdated or unnecessary mechanisms in favor of new, streamlined methods for geometry loading and construction, specifically using the FairGeoSet class and a new LoadAndCreate template method within FairGeoLoader. This improves both the clarity and maintainability of the codebase.

Changes

Files/Paths Change Summary
examples/common/passive/FairCave.cxx Refactored ConstructGeometry to use FairGeoSet, removed inclusion of "FairGeoInterface.h".
fairroot/base/sim/FairModule.h Reordered includes, added new includes, added PrepareGeoSet method, refactored ConstructASCIIGeometry to use FairGeoSet.
fairroot/geobase/FairGeoLoader.h Added template member function LoadAndCreate for creating geometry with FairGeoSet.
templates/project_root_containers/passive/MyCave.cxx Refactored ConstructGeometry to remove explicit FairGeoInterface calls, used LoadAndCreate from FairGeoLoader.
templates/project_stl_containers/passive/MyCave.cxx Updated copyright year, removed FairGeoInterface inclusion, simplified ConstructGeometry using LoadAndCreate.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant FairModule
    participant FairGeoLoader
    participant FairGeoSet

    Client->>FairModule: Call ConstructASCIIGeometry()
    FairModule->>FairGeoLoader: LoadAndCreate<T>()
    FairGeoLoader->>FairGeoSet: Create new instance
    FairGeoSet->>FairGeoLoader: Return instance reference
    FairGeoLoader-->>FairModule: Return FairGeoSet reference
    FairModule->>FairGeoSet: getListOfVolumes()
    FairGeoSet-->>FairModule: Return volume list
sequenceDiagram
    participant Client
    participant MyCave
    participant FairGeoLoader
    participant FairGeoSet

    Client->>MyCave: Call ConstructGeometry()
    MyCave->>FairGeoLoader: LoadAndCreate<MyGeoCave>(GetGeometryFileName())
    FairGeoLoader->>FairGeoSet: Create new instance of MyGeoCave
    FairGeoSet->>FairGeoLoader: Return instance reference
    FairGeoLoader-->>MyCave: Return MyGeoCave reference
    MyCave->>FairGeoSet: Use the loaded geometry

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 - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `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 show all the console.log statements in this repository.` - `@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 as 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. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration 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](https://docs.coderabbit.ai/guides/configure-coderabbit) 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](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
fuhlig1 commented 3 months ago

Looks at first glance reasonable. I will have a closer look next week.

dennisklein commented 3 months ago

Can it be static? Alternatively, a free function?

Shouldn't this just be a member function void FairGeoSet::Prepare()? (Perhaps even better name?)

Otherwise it looks like a factory and then I wouldn't like that it hard-codes a heap object.

dennisklein commented 3 months ago

Shouldn't this just be a member function void FairGeoSet::Prepare()? (Perhaps even better name?)

Perhaps void FairGeoSet::Load() or as a free function void fairroot::Load(FairGeoSet&)¿

(Ideally with an overload which accepts a loader as a function arg)

ChristianTackeGSI commented 3 months ago

Can it be static? Alternatively, a free function?

Shouldn't this just be a member function void FairGeoSet::Prepare()? (Perhaps even better name?)

MGeo->setGeomFile(GetGeometryFileName());

GetGeometryFileName() is a member function of FairModule. So FairGeoSet::Prepare…() should get a reference to the module. Which might be a good thing!

Otherwise it looks like a factory and then I wouldn't like that it hard-codes a heap object.

It is a factory. Yes. Sadly.

GeoInterface->addGeoModule(MGeo);   // takes ownership!

We would first need to change that in some way to handle non-heap objects in a proper way.

Of course we could change the prepare method to take a std::unique_ptr<FairGeoSet> and that way expect a passed in pre-created object.

Putting this Prepare factory in FairModule was the most straight forward refactoring. Because it is called from inside member functions of classes that derive from FairModule.

And I wanted the shortest syntax on the calling side.

PrepareASCIIGeoSet<MyCaveGeoSet>();
// vs.
auto thing = std::make_unique<MyCaveGeoSet>();
PrepareASCIIGeoSet(std::move(thing));
// vs. 
FairGeoSet::PrepareASCIIForModule<MyCaveGeoSet>(*this);
// vs.
auto thing = std::make_unique<MyCaveGeoSet>();
FairGeoSet::PrepareASCIIForModule<MyCaveGeoSet>(std::move(thing), *this);

Maybe in some way my proposed variant is something like a "facade"? You're of course free to do everything that is included in this function on your own (like the current template suggests to do), but you can also use the helper.

dennisklein commented 3 months ago

Can it be static? Alternatively, a free function?

Shouldn't this just be a member function void FairGeoSet::Prepare()? (Perhaps even better name?)

MGeo->setGeomFile(GetGeometryFileName());

Hmm, dang.

ChristianTackeGSI commented 3 months ago

Proposal:

Let's admit, that this is a factory. A factory somewhere between make_unique<T>(…) and vector::emplace(…). It returns a reference (like emplace, basically) to memory managed by GeoInterface.

Granted, this might not be nice/elegant from what we have learned by now. But it resembles quite closely what the refactored code is performing.

Refactoring it should also help us to better understand the surrounding code. (Like why is the calling context different in the templates, the examples and ConstructASCIIGeometry?)

ChristianTackeGSI commented 2 months ago

What about

/**
 * \brief Low level FairGeoSet factory
 */
template<class T, class... Args>
FairGeoSet& FairModule::GeoInterfaceEmplaceGeoSet(Args&&... args)
dennisklein commented 2 months ago

Naming that factored out function feels awkward to me, but I think for a good reason. It is bundling together too many things. After all, this is a template for the user to copy and extend on. Let's take an example where the user wants to setup n geometries. Would we want her to then call

PrepareGeoSet<MyCave>();
PrepareGeoSet<MyDetectorA>();
PrepareGeoSet<MyDetectorB>();

? To me, the proposed function does not compose well.

ChristianTackeGSI commented 2 months ago

ACK generally!

The function is not used in many places (actually only one in the examples, one in each of the two templates, and internally). So this probably isn't a common building block.

And TBH I don't intend it to be a common building block. My current intention is just to make the current "workflow" more obvious, as in "Okay, we have code replication here".

That's also why I am proposing a lengthy and verbose name: So it does not collide with any future, better APIs and is more easily deprecated then.

About the multiple geometries: I think, there will only be one FairGeoSet per FairModule / FairDetector. At least things in the examples look like there is a dedicated GeoSet-class per FairDetector/FairModule class. So that's not really the issue, I'm guessing. Rather, what you mentioned earlier: One probably wants to move the ownership to the FairModule/Detector (probably as a valued member). And maybe then a PrepareGeoSet(FairGeoSet&); would be a much better API.

dennisklein commented 2 months ago

Just one more attempt with yet another API:

void MyCave::ConstructGeometry()
{
    auto& cave = GetGeometryLoader().Load<MyGeoCave>(GetGeometryFileName());
    GetGeometryBuilder().Build(cave);
}
  1. Hide the singleton in the getter
  2. Demo all four user choices: "Loader + GeoSet + File" aka deserialization and Builder aka in-memory initialization
  3. Somewhat still see the composable steps

or, if you want to stick with a single function, maybe LoadAndBuildGeometry<MyGeoCave>(GetGeometryFileName())?

ChristianTackeGSI commented 2 months ago

I like the GetGeometryLoader()! Let's work on that first:

ChristianTackeGSI commented 2 months ago

Just one more attempt with yet another API:

void MyCave::ConstructGeometry()
{
    auto& cave = GetGeometryLoader().Load<MyGeoCave>(GetGeometryFileName());
    GetGeometryBuilder().Build(cave);
}
    Bool_t rc = GeoInterface->readSet(MGeo);
    if (rc) {
        MGeo->create(loader->getGeoBuilder());
    }

I wonder how one would handle the rc forwarding between the two steps? Or should this part be its own "combined" building block?

Maybe some of this state could be moved to the GeoSet? I don't know, really.

Yes: I agree that this error handling is probably not enough or complete or anything. For example MGeo->create() also has a return value, which is just ignored.

As originally said, I wanted to first refactor "what's there" and then improve from there.

dennisklein commented 2 months ago

Just one more attempt with yet another API:

void MyCave::ConstructGeometry()
{
    auto& cave = GetGeometryLoader().Load<MyGeoCave>(GetGeometryFileName());
    GetGeometryBuilder().Build(cave);
}
    Bool_t rc = GeoInterface->readSet(MGeo);
    if (rc) {
        MGeo->create(loader->getGeoBuilder());
    }

I wonder how one would handle the rc forwarding between the two steps? Or should this part be its own "combined" building block?

Maybe some of this state could be moved to the GeoSet? I don't know, really.

Yes: I agree that this error handling is probably not enough or complete or anything. For example MGeo->create() also has a return value, which is just ignored.

As originally said, I wanted to first refactor "what's there" and then improve from there.

In the example I made it would throw.

    Bool_t rc = GeoInterface->readSet(MGeo);
    if (rc) {
        MGeo->create(loader->getGeoBuilder());
    }

This is broken, is it not?

dennisklein commented 2 months ago

Even another variant for inspiration:

void MyCave::ConstructGeometry()
{
    auto& cave = GetGeometryLoader().Load<MyGeoCave>(GetGeometryFileName());
    cave.ConstructGeometry();
}

edit: also it should be MyCaveGeo, no?

ChristianTackeGSI commented 2 months ago

Even another variant for inspiration:

void MyCave::ConstructGeometry()
{
    auto& cave = GetGeometryLoader().Load<MyGeoCave>(GetGeometryFileName());
    cave.ConstructGeometry();
}

What choice of the user is cave.ConstructGeometry(); representing? "To build or not to build" ;-) In that case I would vote for moving it into the previous line. ;-)

I liked the explicit GeoBuilder reference. … also it is already included in the GeoLoader…

edit: also it should be MyCaveGeo, no?

Naming, naming. You know it's a mess. ;-)

This is broken, is it not?

As always… Trying to refactor it, before we fix it. So that we only need to fix it in one place.

Yet another inspiration…

void MyCave::ConstructGeometry()
{
    auto cave_on_heap = std::make_unique<MyCaveGeoSet>("Cave17");
    auto& cave = GetGeometryLoader().LoadAndConstruct(std::move(cave_on_heap),
                                                      GetGeometryFileName());
}
dennisklein commented 2 months ago 
void MyCave::ConstructGeometry()
{
    GetGeometryLoader().LoadAndConstruct<MyCaveGeo>(GetGeometryFileName(), "Cave17");
}

btw, https://github.com/FairRootGroup/FairRoot/blob/99be5f48d4bfed7ee7179524c07bc49a6b37a5c5/fairroot/geobase/FairGeoSet.h#L113

So, a loader consists of an interface (the actual thing that performs the loading) and a builder. The builder, however, is used as part of a create action. All this is done to construct a geometry. Let's add init somewhere and we have used all possible synonyms. And all of this happens outside of the constructor the language provides for initialization of an object :confused:

edit: Sticking with the existing terminology, it should be

GetGeometryLoader().ReadAndCreate<MyCaveGeo>(GetGeometryFileName(), "Cave17");

no?

ChristianTackeGSI commented 2 months ago

So we're at

GetGeometryLoader().LoadAndConstruct<T>(TString, args...)
// vs.
GetGeometryLoader().ReadAndCreate<T>(TString, args...)

I don't care much.

ChristianTackeGSI commented 2 months ago

So, now we have

GetGeometryLoader().LoadAndConstruct<T>(const char *, args…);

What do you think?