R3BRootGroup / R3BRoot

Framework for Simulations and Data Analysis of R3B Experiment
https://github.com/R3BRootGroup/R3BRoot/wiki
GNU General Public License v3.0
18 stars 104 forks source link

Rewrite R3BFileSource #871

Closed YanzhaoW closed 1 year ago

YanzhaoW commented 1 year ago

The new R3BFileSource2 has been tested with NeulandMap2CalPar and NeulandDigitizer.

Feel free to comment below if you have more suggestions. If there is no problem for the new file source, we could consider replacing the old file source with it in the future.


Checklist:

ryotani commented 1 year ago

Hi, @YanzhaoW. Just out of curiosity, what is the motivation for the new class? Is it mostly to modernise the way of writing codes, or are there new functionalities included? What shall we have to do if we use FileSource2? You changed the namespace to R3B, does it affect other codes also?

YanzhaoW commented 1 year ago

Hi, @ryotani. Thanks for your comment.

Kind of both. The main motivation is to rewrite it in a clean way. The new version is way shorter and easier to understand.

You could see the difference by counting the lines of code: R3BFileSource.h: 230 R3BFileSource.cxx: 1028 R3BFileSource2.h: 123 R3BFileSource2.cxx: 424

Actually in the beginning, what really set my teeth on edge is the strange interfaces of the old class. If I want to use multiple root files as the data source, I have to add the first file in the beginning and then add the rest of files together:

    auto source = new R3BFileSource(TString::Format(inFilePattern, RunID));
    source->SetRunId(999);
    for (int i = 1; i <= nSubruns; i++)
    {
      source->AddFile(TString::Format(inFilePattern, RunID+i));
    }

This is strange. So I went to the source code to see whether there is an option to add them together. And what I found is a huge mess. :D So instead of wasting time on adding more mess on it. I thought it's better to rewrite it. So, with the new FileSource, you could just do:

auto source = new R3BFileSource2(); // DON't USE NEW
    for (int i = 0; i <= nSubruns; i++)
    {
      source->AddFile(TString::Format(inFilePattern, i)); // Use fmt::format()
    }

And the runID will be automatically detected and set from the root file. So no need to set the runID. There is also better error handling from the new class.

What shall we have to do if we use FileSource2? You changed the namespace to R3B, does it affect other codes also?

The only thing you need to do is just to change the name R3BFileSource to R3BFileSource2. They should have the same interfaces. If not, please tell me and I will add it to the new class.

The namespace changing is only for the functions I made few months ago, such as root_owned<> or make_rootfile<>. I used the namespace r3b. But to make it consistent to other file names, R3B should be better. There is no namespace for the R3BFileSource2.

jose-luis-rs commented 1 year ago

Nice work @YanzhaoW, if there are no objections, I will do the merge for testing it.

YanzhaoW commented 1 year ago

Hi @jose-luis-rs Could you merge this PR?