Implementation of Parameter finder for Tprev/Tnext calculation

[leandrofloyd]

Implementation of Parameter finder for Tprev/Tnext calculation. The parameter is the time offset between the hit recorded in SAMPv and the same hit accepted for SAMPMSv. The time difference is due to the needed time for the Trigger Logic to approve the hit as a Master Start.

Checklist:

• [x] Rebased against dev branch
• [x] My name is in the resp. CONTRIBUTORS/AUTHORS file
• [x] Followed the pull request guidlines and the Git workflow
• [x] Followed the seven rules of great commit messages

[klenze]

Semi-off topic:

It seems that FairRoot now uses factory classes to handle parameter classes.

I confess that was not on my bingo sheet. Of course there is nothing specific to the parameter class in @leandrofloyd factory, but as C++ is famously a feature-starved language which lacks template support there is no way FairRoot could provide a generic way to handle that.

(My solution in FairRoot's place would have been to either use the ROOT dictionaries to try to look up a class by its string name, or provide a macro to register a mapping of name-string to factory function (which would be a simple lambda expression), and have all Parameter classes call that. But I guess creating globals is a must for spiritual compatibility with ROOT, and having the users copy-paste their factories makes them feel more productive.)

@leandrofloyd: Please try not to get bogged down too much in this parameter circus. At the end of the day, the offset is just some signed integer. There is nothing wrong with having a first prototype where you pass the offset in the constructor of your tprev/tnext calculator, and then leave the boilerplate code needed to encrypt that integer into .root files to the proponents of FairRoot flavor of parameters.

[leandrofloyd]

If there are no more comments, this is ready to be merged.

[YanzhaoW]

I will check your code in detail this week and leave some comments. 😆

[ajedele]

Dear @YanzhaoW, would it be possible to speed up your review? I understand you may not agree with the way Leandro is implementing the parameter. However, this is the standard way to implement a parameter into R3BRoot. If you have an issue with that, please contact the framework developers and not the functionality implementers. In addition, I am waiting for this code to be submitted, so that I can see if it has an impact onto my PSP calibrations and whether or not I need to build this into the calibration. I am using a forked version of R3BRoot that has my code implemented, so it's not as simple as using Leandros branch in the meantime

[YanzhaoW]

@ajedele Sure, I will do that later today.

I am using a forked version of R3BRoot that has my code implemented, so it's not as simple as using Leandros branch in the meantime

You can try that now without waiting it to be merged to our dev branch. Git is extremely flexible to integrate other people's contribution.

If you are in a forked repository and still using the latest dev branch, then do:

git remote add ${name} ${repo_url}
git fetch ${name}
git checkout ${branch_name}

If you are in a fork repository and even using different branch, then do:

git remote add ${name} ${repo_url}
git fetch ${name}
git rebase ${name}/${branch_name}

Let me know if there is any problem for this.

[klenze]

@ajedele

In addition, I am waiting for this code to be submitted, so that I can see if it has an impact onto my PSP calibrations and whether or not I need to build this into the calibration.

From my understanding, the tnext/tprev calculation is not yet in, so I doubt this PR will help you very much with that.

@YanzhaoW: I would be reluctant to run git rebase to apply specific changes unless your feature branch is already rebased to dev.

@leandrofloyd:

The container factory thing default-initializes the offset parameter if it does not exist?

I would argue that this is not the behavior we want for parameters. If a task needs a parameter and the parameter was not provided, it should crash, not use some default value.

Also, zero is a terrible default value for "invalid parameter", it will give you tprev/tnext which are not quite correct, but might look correct. If you used an int64, I would use 1<<63 or so. As you are using a double for the parameter, the obvious choice is NAN.

If fMinStatistics is not reached, then the parameter is quietly not changed. This does not seem very user friendly to me. If you did not get enough events, the task has failed. Let the user know. LOG(fatal), for example. Simply writing out whatever the parameter was before (possibly 0.0) is not what I would expect.

I advocate for making the histogram a part of the tprev/tnext calculation task and keep it running at all times, writing it out with the tree file. Anyone who uses tprev/tnext could then quickly check that the offset is correct and did not change between runs. (In fact, during R3BTprevTnext::FinishTask(), one could automatically verify that the peak of that histogram is roughly equal to the parameter, and warn the user otherwise. As a bonus, we would not need to different tasks, but just pass a bool calibrate=1 to the constructor to indicate that we want a new calibration.)

[YanzhaoW]

I would be reluctant to run git rebase to apply specific changes unless your feature branch is already rebased to dev.

Why is that?

The container factory thing default-initializes the offset parameter if it does not exist?

Unfortunately yes. That's for every parameter dealt by FairRoot. However, in my PR #890, I use par->hasChanged() to see whether the parameter is just default initialised or actually read from the root file.

[klenze]

Why is that?

Say you rebase your featureA branch on a featureB branch from a colleague for testing. It works. A different version, featureB' gets merged into main. You continue working on featureA, adding commits. Then you want to do a PR, so you rebase on main. That rebase fails now because there is a conflict between featureB and featureB'. So in effect you have to figure out how to unrebase featureB. This is certainly doable, cherry-pick or something. However, it is not part of git 101.

So telling someone "just rebase on featureB" is a bit like telling them someone how to start with a paraglider in that it will likely put them into a situation where they will urgently need further advice. This can be fine (people learn advanced git or paragliding all the time), but one should point out the likely roadblocks downstream.

Unfortunately yes. That's for every parameter dealt by FairRoot.

That seems like a terrible idea, in particular with people using plausible-seeming default values (e.g. 0.0 here).

In fact, I don't understand the rationale behind the factories at all. Why not let the task responsible for creating valid calibrations instantiate the parameters and register them? What use case is filled by having parameters in an invalid default state instead of returning nullptr? If a task requires a parameter, that parameter should be defined exactly once in the parameter files. Not zero times, not multiple times, once. Anything else should throw an error.

[YanzhaoW]

Say you rebase your featureA branch on a featureB branch from a colleague for testing. It works. A different version, featureB' gets merged into main. You continue working on featureA, adding commits. Then you want to do a PR, so you rebase on main. That rebase fails now because there is a conflict between featureA and featureB'.

I think the correct way to do this is to use git rebase --onto main featureB your_branch . With this you could reverse the rebase of featureB and then rebase it again to the main. Then you don't need to worry about the conflict between featureA and featureB.

In fact, I don't understand the rationale behind the factories at all.

Me neither. Actually in my PR #890 , I don't have the parameter factory at all and it still works. The design of FairRootManager is an over-complicated mess and most of its ugly parts haven't been touched at least for a decade.

[leandrofloyd]

I made some proposed changes.

The ones I decided to not do were because: 1 - The task stopped working properly (this is the case of changing R3BSamplerMappedData with auto); 2 - The change is not something that really matters, like the inline initialization where you just change the position where the commas are.

I added //NOLINT to some parts of the code, where, for example, I was asked to remove a destructor but when I did that clang-tidy complained that a destructor was missing. In this case, or I use //NOLINT or I don't remove the destructor, the option was made by the use of //NOLINT.

[jose-luis-rs]

Say you rebase your featureA branch on a featureB branch from a colleague for testing. It works. A different version, featureB' gets merged into main. You continue working on featureA, adding commits. Then you want to do a PR, so you rebase on main. That rebase fails now because there is a conflict between featureA and featureB'.

I think the correct way to do this is to use git rebase --onto main featureB your_branch . With this you could reverse the rebase of featureB and then rebase it again to the main. Then you don't need to worry about the conflict between featureA and featureB.

In fact, I don't understand the rationale behind the factories at all.

Me neither. Actually in my PR #890 , I don't have the parameter factory at all and it still works. The design of FairRootManager is an over-complicated mess and most of its ugly parts haven't been touched at least for a decade.

@kresan, please, could you comment on the consequences of removing the parameter factory?

[YanzhaoW]

I made some proposed changes. The ones I decided to not do were because: 1 - The task stopped working properly (this is the case of changing R3BSamplerMappedData with auto); 2 - The change is not something that really matters, like the inline initialization where you just change the position where the commas are.

Thanks for the changes. Replacing R3BSamplerMappedData* with auto* shouldn't be a problem. Is it a compile error or a runtime error? And what is the error message you got?

Except this part, I think everything else looks good enough.

[YanzhaoW]

Ok, here is the cause for this abnormality if anyone is interested to know:

The cause is the subtraction of two unsigned integer. If we have two unsigned integer:

auto const var1 = unsigned int {2};
auto const var2 = unsigned int {5};
auto const sub = var1 - var2;

the result sub is not -3 but rather 2^32 - 3 because the subtraction of two unsigned int is still unsigned int. To fix this issue, one has to change the time type defined in R3BSamplerMappedData to be int instead of unsigned int.

So here is a general rule: If a value can be subtracted to become a negative value, its type must not be unsigned int.

[leandrofloyd]

Everything has been fixed. Ready to be merged.

[kresan]

Container factory is a static object in the library, which takes care that the parameter containers are automatically created before they are being accessed, so at loading of the library. Hope now the consequences of its removal are obvious.

[klenze]

@YanzhaoW: yupp, that's how integers work. The subtraction instruction is identical for signed and unsigned data, so it would also be permissible to just reinterpret the difference as signed, but I think casting the operands is more defensive. Speaking of wraparounds, upexps/land_common/trloii.spec indicates that the trloii scalers are 32 bit numbers (@bl0x: correct?). In that case I would prefer using int32_t for the subtraction, because it also takes care of the overflows (which should happen once per 43 seconds with 32 bits and 10ns clock) correctly. Pedants might prefer to store the times themselves as uint32_t and just make the difference signed.

[jose-luis-rs]

Please, test the code and if the change "int" -> "int32_t" is finally needed for the analysis, open a new PR with the modifications.