R3BRootGroup
New R3BFootCal2Hit for FOOT detectors
New r3BFootCal2Hit task. From the point of view of the clustering and position reconstruction the task did not change. The most important modification is the eta-energy calibration. The task include both calibration for S522 and S509. It possible to use one or the other with SetExpId(). This task allow to obtain a better separation of the charges in the FOOT detectors, in particular, if we look at the at the correlations between the 2 FOOT inside the vacuum chamber (charge 3, 4, 5, 6) for C beam (S522). Still charge 1 and 2 are not well defined since they are covered with noise that set a very low value of energy. This pull also include the possibility to have the eta parameter at the hit level and small modifications of the R3BFootMapped2StripCal.
Andrea Lagni and Antoine Barriere
Mention any issue this PR is resolved or is related to.
Checklist:
- [x] Rebased against
devbranch - [x] My name is in the resp. CONTRIBUTORS/AUTHORS file
- [x] Followed the seven rules of great commit messages
@YanzhaoW the clang-tidy seems to give errors related to FairRoot classes, are you sure it is well configured?
I also see that one could have a lot of problems to understand the warnings/errors, for instance:
DiagnosticName: misc-const-correctness Level: Warning
- BuildDirectory: /__w/R3BRoot/R3BRoot/build/ssd
DiagnosticMessage:
FileOffset: 22798
FilePath: /__w/R3BRoot/R3BRoot/ssd/calibration/R3BFootStripCal2Hit.cxx
Message: statement should be inside braces
Replacements:
- {FilePath: /__w/R3BRoot/R3BRoot/ssd/calibration/R3BFootStripCal2Hit.cxx, Length: 0, Offset: 22798, ReplacementText: ' {'}
- {FilePath: /__w/R3BRoot/R3BRoot/ssd/calibration/R3BFootStripCal2Hit.cxx, Length: 0, Offset: 22808, ReplacementText: ' }'}
What does it mean?
I have the impression that the clang-tidy is not under control because your last PR #728 also fails. Should we disconnect it meanwhile?
@jose-luis-rs Hi, please check the clang-tidy comments in "File Changed" tag. Or if you want to check the output, please check under the step "clang-tidy check" and all the warnings inside.
Yes, I haven't finished my PR "#728" and it's still in progress. The clang-tidy failure is much expected.
Also when you click the "Details", it redirects you to the console output. In this case, please click the summary on the top left corner to see the warning the messages.
From the warning messages I saw, they are well justified.
For some reasons, github actions only display maximal 10 warnings. I will see what I can do about it. But like I said, you can find all the warnings from "clang-tidy check" step.
@jose-luis-rs I will fix the warnings from included files as soon as possible.
582 warnings sound too much: "Clang-tidy generates 582 warnings! Please fix them before being merged."
Some of them could be real bugs, but others like the usual getters: Int_t GetDetId() const { return fDetId; }
or default parameter values for standard detector configurations like:
- BuildDirectory: /__w/R3BRoot/R3BRoot/build/ssd DiagnosticMessage: FileOffset: 23839 FilePath: /__w/R3BRoot/R3BRoot/ssd/calibration/R3BFootStripCal2Hit.cxx Message: 200 is a magic number; consider replacing it with a named constant Replacements: [] cannot be considered as bugs. They shoud be discarded.
I will fix those warnings from included files. But even with this, warnings could still go to few hundreds. In this case, it only means the pushed code is badly written. We already have too much bad code in our dev branch and I suppose nobody wants more. So my suggestion is look at the warning message and fix the code. Those message is very easy to understand.
Magic numbers such as "200", "1.7" or "0.1" are not bugs only now. And if different people in few years look back to these numbers, I hope they could understand them as well when they change it. If they don't, well, I wish we could have good luck then.
Fixing this issue is very easy, just use a constant variable, as suggested by clang-tidy:
auto const PaddleNumber = 200;
With this, we immediately know this 200 is the number of paddles not something else. And this can also be easily understood with people in few years, even without comments or documents.
Same goes for getters, just add [[nodiscard]] and const if necessary.
Where should we define this line?
auto const PaddleNumber = 200;
It depends on how long we need it. If we need it the whole program, define it as a global constant variable ( must be inside a namespace). If we need it for the lifetime of objects from a class, make it as a (static) constant member variable. If just used once for a function input, define them in the beginning of the function body or before the usage.
If you want it to be easy, just put it before the usage.
Usually these parameters are defined in-flight taking the right values from parameter containers, so the initial value is redundant.
Usually these parameters are defined in-flight taking the right values from parameter containers, so the initial value is redundant.
Could you give me a specific example in this case, where a magic number must be needed?
This is another bug that shouldn't be in the list:
FilePath: /__w/R3BRoot/R3BRoot/r3bdata/footData/R3BFootHitData.cxx Message: '''Int_t'' and ''Double_t'' may be implicitly converted: ''Int_t'' (as ''int'') -> ''Double_t'' (as ''double''), ''Double_t'' (as ''double'') -> ''Int_t'' (as ''int'')' Replacements: []
This is another bug that shouldn't be in the list:
FilePath: /__w/R3BRoot/R3BRoot/r3bdata/footData/R3BFootHitData.cxx Message: '''Int_t'' and ''Double_t'' may be implicitly converted: ''Int_t'' (as ''int'') -> ''Double_t'' (as ''double''), ''Double_t'' (as ''double'') -> ''Int_t'' (as ''int'')' Replacements: []
This isn't in the list and not a warning. A warning has "Level: warning" in the end.
Usually these parameters are defined in-flight taking the right values from parameter containers, so the initial value is redundant.
Could you give me a specific example in this case, where a magic number must be needed?
Magic numbers are only there to ensure that the code does not crash if a first user runs it without loading the corresponding parameter files (.root or .par). Anyway, it is better to have all these parameters defined in a specific file to avoid inconsistencies between classes of the same detector.
This is another bug that shouldn't be in the list: FilePath: /__w/R3BRoot/R3BRoot/r3bdata/footData/R3BFootHitData.cxx Message: '''Int_t'' and ''Double_t'' may be implicitly converted: ''Int_t'' (as ''int'') -> ''Double_t'' (as ''double''), ''Double_t'' (as ''double'') -> ''Int_t'' (as ''int'')' Replacements: []
This isn't in the list and not a warning. A warning has "Level: warning" in the end.
Ah, sorry, I didn't know about this difference. Thanks.
Usually these parameters are defined in-flight taking the right values from parameter containers, so the initial value is redundant.
Could you give me a specific example in this case, where a magic number must be needed?
Magic numbers are only there to ensure that the code does not crash if a first user runs it without loading the corresponding parameter files (.root or .par). Anyway, it is better to have all these parameters defined in a specific file to avoid inconsistencies between classes of the same detector.
Could you give me a concrete example, like some lines of code? And one more thing is all the floating point values are ignored.
Usually these parameters are defined in-flight taking the right values from parameter containers, so the initial value is redundant.
Could you give me a specific example in this case, where a magic number must be needed?
Magic numbers are only there to ensure that the code does not crash if a first user runs it without loading the corresponding parameter files (.root or .par). Anyway, it is better to have all these parameters defined in a specific file to avoid inconsistencies between classes of the same detector.
Could you give me a concrete example, like some lines of code? And one more thing is all the floating point values are ignored.
These lines for instance: https://github.com/R3BRootGroup/R3BRoot/blob/883b31a2b8ed5995811b47e25f71bdf1d2971711/alpide/calibration/R3BAlpideCal2Hit.cxx#L52
https://github.com/R3BRootGroup/R3BRoot/blob/883b31a2b8ed5995811b47e25f71bdf1d2971711/alpide/calibration/R3BAlpideCal2Hit.cxx#L112
Usually these parameters are defined in-flight taking the right values from parameter containers, so the initial value is redundant.
Could you give me a specific example in this case, where a magic number must be needed?
Magic numbers are only there to ensure that the code does not crash if a first user runs it without loading the corresponding parameter files (.root or .par). Anyway, it is better to have all these parameters defined in a specific file to avoid inconsistencies between classes of the same detector.
Could you give me a concrete example, like some lines of code? And one more thing is all the floating point values are ignored.
These lines for instance:
https://github.com/R3BRootGroup/R3BRoot/blob/883b31a2b8ed5995811b47e25f71bdf1d2971711/alpide/calibration/R3BAlpideCal2Hit.cxx#L52
https://github.com/R3BRootGroup/R3BRoot/blob/883b31a2b8ed5995811b47e25f71bdf1d2971711/alpide/calibration/R3BAlpideCal2Hit.cxx#L112
integers like "1" or "0" will not be marked as magic numbers, I think. Magic numbers are like "99", "13", "34" or "1000".
wrt magic numbers, I find myself again at a moderate position. If you use magic constants as the bounds of a for loop, that sounds clearly evil. If, on the other hand, you use them as default parameters, that sounds much less sinister. I don't think
class Foo
{
public:
static const int cDefaultNumberOfModules=2;
Foo(int numModules=cDefaultNumberOfModules);
}
increases readability over Foo(int numModules=2). (It may be worthwhile if the same constant is used in multiple places, though).
Integers like "1" or "0" will not be marked as magic numbers, I think. Magic numbers are like "99", "13", "34" or "1000".
compromise: build up all magic integers out of expressions of zeros and ones so the compiler won't complain. :-)
I will fix those warnings from included files. But even with this, warnings could still go to few hundreds. In this case, it only means the pushed code is badly written. We already have too much bad code in our dev branch and I suppose nobody wants more. So my suggestion is look at the warning message and fix the code. Those message is very easy to understand.
Hi,
I would like to write few comments about the code:
1- This code is nothing new. The structure and 85% of the code is exactly as the previous one. So there is no new "badly" written code, but is the same as before. 2- Regarding the clang tidy test I thinks that if we push all R3BRoot trough the clang-tidy test a huge amount of things won't pass this test. This means that after this test was implemented, one should also care of rewrite tasks in the proper way in order to pass the test, and this can take a lot of time. And sometimes someone needs some features in the code to be implemented fast, even if was not written perfectly. 3- At the moment I don't have a lot of time to works in the corrections of the warnings. The FOOT task are used only by s522 and s509, so all these warning won't affect anyone.
Best, Andrea Lagni
Like I said in the last R3B analysis meeting, clang-tidy only checks the changed code. It WILL NOT give any warnings on existing code.
This code is nothing new. The structure and 85% of the code is exactly as the previous one. So there is no new "badly" written code, but is the same as before.
If 85% of the code is exactly same as the previous one, they shouldn't be shown in the Github page under 'Files changed' and clang-tidy wouldn't check them.
Regarding the clang tidy test I thinks that if we push all R3BRoot trough the clang-tidy test a huge amount of things won't pass this test. This means that after this test was implemented, one should also care of rewrite tasks in the proper way in order to pass the test, and this can take a lot of time.
I'm not sure what you mean. But if I understand you correctly, no, Clang-tidy only checks the changed code. One doesn't need to rewrite already existing bad code to pass the test. It only checks the quality of new implemented features.
And sometimes someone needs some features in the code to be implemented fast, even if was not written perfectly.
Uncooked code should not be merged to dev branch. If someone need your feature fast, they could pull the branch directly from your own forked repo. Bad code merged into the dev branch will effect everyone including those people who don't need your features. One of examples is PR #814. It ran fine on califa but crashed the program for NeuLAND. And I spent a whole day to correct it just by changing > to >=. If we keep merging the bad code into the dev branch, we just keep wasting other people's time sooner or later.
At the moment I don't have a lot of time to works in the corrections of the warnings. The FOOT task are used only by s522 and s509, so all these warning won't affect anyone.
We could just keep the PR here. If you don't have time at the moment, fix them when you have time. Again, if someone need your feature urgently, please tell them to pull your branch from your forked repo.
Last, don't make a PR on your forked dev branch. Create a new branch for the new feature.
Like I said in the last R3B analysis meeting, clang-tidy only checks the changed code. It WILL NOT give any warnings on existing code.
This code is nothing new. The structure and 85% of the code is exactly as the previous one. So there is no new "badly" written code, but is the same as before.
If 85% of the code is exactly same as the previous one, they shouldn't be shown in the Github page under 'Files changed' and clang-tidy wouldn't check them.
Regarding the clang tidy test I thinks that if we push all R3BRoot trough the clang-tidy test a huge amount of things won't pass this test. This means that after this test was implemented, one should also care of rewrite tasks in the proper way in order to pass the test, and this can take a lot of time.
I'm not sure what you mean. But if I understand you correctly, no, Clang-tidy only checks the changed code. One doesn't need to rewrite already existing bad code to pass the test. It only checks the quality of new implemented features.
And sometimes someone needs some features in the code to be implemented fast, even if was not written perfectly.
Uncooked code should not be merged to dev branch. If someone need your feature fast, they could pull the branch directly from your own forked repo. Bad code merged into the dev branch will effect everyone including those people who don't need your features. One of examples is PR #814. It ran fine on califa but crashed the program for NeuLAND. And I spent a whole day to correct it just by changing
>to>=. If we keep merging the bad code into the dev branch, we just keep wasting other people's time sooner or later.
I think that this problem was solved in 5min, but for this kind of issue I recommend to use the "issues" section because, likely, you get the solution faster.
At the moment I don't have a lot of time to works in the corrections of the warnings. The FOOT task are used only by s522 and s509, so all these warning won't affect anyone.
We could just keep the PR here. If you don't have time at the moment, fix them when you have time. Again, if someone need your feature urgently, please tell them to pull your branch from your forked repo.
Last, don't make a PR on your forked dev branch. Create a new branch for the new feature.
I think that this problem was solved in 5min, but for this kind of issue I recommend to use the "issues" section because, likely, you get the solution faster.
Ok, yes, we could try that next time.
So there is no new "badly" written code, but is the same as before.
@AndreaLagni: did or did you not write the following change:
ssd/calibration/R3BFootStripCal2Hit.cxx
- fFootCalData = dynamic_cast<TClonesArray*>(rootManager->GetObject("FootCalData"));
+ fFootCalData = (TClonesArray*)rootManager->GetObject("FootCalData");
Also,
fZ4_15 = new TF1("fZ4_15"," 397.864 -3004.03*TMath::Power(x,1) + 10628.8*TMath::Power(x,2)
+ 2035.04*TMath::Power(x,3) -110125*TMath::Power(x,4) + 308386*TMath::Power(x,5)
-388480*TMath::Power(x,6) + 236311*TMath::Power(x,7) -55772*TMath::Power(x,8)",0.,1.);
It is hard to overstate how opposed I am to the inclusion of the above. Putting a few kilobytes of parameters into the source file (and as strings parsed by CINT as functions, adding insult to injury) should not be anywhere near our overton window.
If you want your PR to be merged, I would suggest starting from the scratch. Find an experienced C++ coder and ask them to help you with your design. Depending on whom you ask, you might end up storing your calibration in yaml and creating std::function objects during startup or storing the parameters in custom TObjects made up of one-dimensional TArrays, out of which you construct C-arrays of TF1*. Either would be an improvement.
Cheers, Philipp
I agree that we will need a parameter container to manage these parameters, but I am not sure about the first option "yaml and creating std::function" because I have the feeling that it will not fulfill one of our requirements. -> change parameters in-flight. At least I hope that your implementation based on yaml files allows to dump the info into Root files automatically, but it does not matter, because I guess that one can also do a macro to read your yaml parameter container and fill the other options.
I agree that we will need a parameter container to manage these parameters, but I am not sure about the first option "yaml and creating std::function" because I have the feeling that it will not fulfill one of our requirements. -> change parameters in-flight. At least I hope that your implementation based on yaml files allows to dump the info into Root files automatically, but it does not matter, because I guess that one can also do a macro to read your yaml parameter container and fill the other options.
Hi @jose-luis-rs, could you tell me more about this changing parameters in-flight? Do you mean changing the parameters during the running of the program?
I agree that we will need a parameter container to manage these parameters, but I am not sure about the first option "yaml and creating std::function" because I have the feeling that it will not fulfill one of our requirements. -> change parameters in-flight. At least I hope that your implementation based on yaml files allows to dump the info into Root files automatically, but it does not matter, because I guess that one can also do a macro to read your yaml parameter container and fill the other options.
Hi @jose-luis-rs, could you tell me more about this changing parameters in-flight? Do you mean changing the parameters during the running of the program?
Yes, exactly. You can find the explanations in Gabriel's presentations from analysis meetings, R3B collaboration meetings and also from our last R3B workshop.
Yes, exactly. You can find the explanations in Gabriel's presentations from analysis meetings, R3B collaboration meetings and also from our last R3B workshop.
Ok, I see. But do you know whether Gabriel will keep working on this? I don't think current method of reading txt file is a long-term solution.
I don't think current method of reading txt file is a long-term solution.
Gabriel is busy with his analysis/thesis implementing neural network algorithms for CALIFA.
During beam times (online analysis) the txt files are very useful to change the values of some parameters used for tracking (like FRS settings: Brho, position selections, basic data corrections to improve resolutions, ...). In this case the users always prefer to have the txt file opened in a screen to change the values directly and run the online to see the effects.
For nearline analysis depends on the detectors, for instance, the MWPCs are very stable and the number of parameters is small. So one can use both, txt or root files, because the parameters will be the same for all the runs. In the case of ionization chambers we would need to change the parameter setting for each run (if they last less than 2-3 hours). In this case, if we have all the parameters in a root file indicating the range of use for each parameter setting as a function of the timestamps, the users only need to load this root file from their macros and run the code to produce the new data, R3BRoot should do all the job alone. This is already implemented in our R3BParams repositories, but at the moment we are still calibrating the detectors for each run/setting. After this step, we should start to produce the root file with all the parameter settings.