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

TwiM drift time calib fixed, suppress unnecessary cout #866

Closed ryotani closed 1 year ago

ryotani commented 1 year ago

The drift time calibration for TwiM in s467/s444 (2020) has to be independent of the standard configuration since two MDPP16 were used for section 0 for those cases. The mapping of the time reference was wrong previously, and the R3BCoarseTimeStitch has been removed since it was causing fake reconstructions. The code has been simplified.

Describe your proposal.

Mention any issue this PR is resolved or is related to.


Checklist:

ryotani commented 1 year ago

Hi, I need your help @YanzhaoW @klenze. This is way beyond my expertise in writing C++ code. Initially, I wanted to change the channel of Time reference for the drift time correction, resulting in me having to almost rewrite the code since CI cannot be happy with any modifications. It's not my code originally, but previous authors were using fE[section][mult][ChId] and a similar multi-dimensional array fDT[][][]. I tried to define proper classes as CalSection, CalAnode and seems working as expected. But I cannot find the solution to answer the message as:

assigning newly created 'gsl::owner<>' to non-owner 'std::array<R3BTwimMapped2Cal::CalSection , 4>::value_type' (aka 'R3BTwimMapped2Cal::CalSection ') [cppcoreguidelines-owning-memory]

Could you advise me to solve this? Other comments might be able to be accessed by me.. But I would say this clang-tidy creates messes for people as they want to fix only a few lines, since clang-format will change many lines then clang-tidy will start pointing out issues caused not by the latest contributors....

YanzhaoW commented 1 year ago

Hi, @ryotani

No problem. The warning (yes, the warning message is not clear) is caused by the usage of new operator. Instead of using new operator, you should create the resource by using std::make_unique(). By this way, you can get rid of unreleased memory or deleting a pointer twice. Google unique pointer of C++ or search it in youtube and you could get a very good understanding of why we need it.

But I will check your code and give some reviews tomorrow. If you have more questions, we could discuss tomorrow.

ryotani commented 1 year ago

Sorry for making many attempts for the pull requests. I think it passed all the tests now. Should I rebase to the latest dev branch, or can it be merged with no problem?

YanzhaoW commented 1 year ago

Yes, of course. If there is no other issue and you are ready for the merging, please rebase to the latest dev branch:

# at your PR branch and your dev branch is the latest
git rebase dev

And don't forget to squash all commits into one:

git rebase -i HEAD~${Number of commits to be squashed}
ryotani commented 1 year ago

Thanks for the help @YanzhaoW. I rebased to the latest commit in the dev branch. Then squash merged two commits.

ryotani commented 1 year ago

Hi @jose-luis-rs

This PR is also ready to be merged. Thanks.

jose-luis-rs commented 1 year ago

Thanks @ryotani