My recommendations for the further development of R3BRoot

[janmayer]

Hi all. During the last years, I have become very familiar with R3BRoot. These are my recommendations for improving the code base that I didn't find the time to fix myself. Some are unnegotiable must-dos others are just nice to have (and everything in between).

Remove the connection to the macro directory

At some point, the macro directory was separated out due to frequent changes. However, it contains tests and is required in the building process. I would argue that this disruption to the automated testing, the commit system, and especially the user experience are no longer justified. There also seem to be several macros no longer used or maintained. Thus, I propose:

• Move the tests to the respective detector directories
• Move geometry creating macros to geometry/macros
• Delete old code
• Remove the macro directory dependence in the main CMakeLists.txt
• Move (few!) files that are still useful to the main repository

Host UCESB and UPEXPS on GitHub

There is something mystical about UCESB & Co. Few people know how to work with it, and even fewer know how it works. Yet, they are essential components of R3BRoot.

I see it as an absolute necessity to host these two packages on GitHub. The current maintainers should, of course, have administrative access.

• Create and push to R3BRootGroup/UCESB
• Create and push to R3BRootGroup/UCEXPS
• Set correct permissions for current maintainers
• Integrate GitHub Actions for automated checks & tests
• Apply coding best practices, e.g., code formatting
• Move or create documentation on GitHub

Consolidate documentation

The documentation for R3BRoot is scattered over several systems:

• https://wiki.r3b-nustar.de/
• https://www.r3broot.gsi.de/
• https://github.com/R3BRootGroup/R3BRoot/wiki/
• https://r3bproject.gsi.de/projects/r3b-analysis/
• https://forum.gsi.de/index.php?t=index&cat=40&rid=0

Some of it also refers to obsolete versions (e.g., my old installation instructions for FairSoft). In my opinion, the code documentation should be as close to the code as possible. Thus, I recommend using the build-in wiki of GitHub. It is easy to use, everything is written in Markdown, and everyone has or can be given access.

I started by creating documentation on Installation and Tasks. I would ask everyone with knowledge of a specific system to write it down there.

This GitHub wiki should be restricted to code and its usage; experiment or detector specific information should probably stay in the R3B-Nustar-Wiki

Note that this will probably make www.r3broot.gsi.de obsolete. In my opinion, this is a good thing. Once it is shut down and the URL points to this repository, it is one less thing to administer and to keep up-to-date.

• Move and update relevant documentation from www.r3broot.gsi.de to GitHub
• Move and update relevant documentation that refers to R3BRoot coding or usage from the R3B-Nustar-Wiki to GitHub
• Have a plan to write documentation on the relevant parts, especially for new users.
• Discontinue www.r3broot.gsi.de.
• Discontinue the forum, use GitHub issues or the OpenProject instead.

Clean R3BRoot Code

There is a lot of duplicated code in R3BRoot currently. One is internal duplication, which should be easy to remove with a simple abstraction. The best example for this are the dozen different fiber detectors fi*. They are almost all virtually identical in functionality. The only difference is the name. I think there should be a way to consolidate them into a template, generate the code during compilation, or create a class that simply takes the names as strings.
Some code in FairRoot was copied and renamed to R3BRoot on project creation. It might be a challenge to figure out, but some code can just be deleted and the FairRoot version used instead.

• De-duplicate detectors, especially all the fibers.
• Identify, test, and then remove elements that exist in FairRoot where an R3BRoot-owned version is unneccsary. Start with R3BMCStack and R3BMCTrack.

I would also argue that the base directory of R3BRoot is overfilled, to the point that one loses overview. It will get significantly better once the fibers are cleaned up. Moving all detectors into a /detectors/ folder would then significantly increase Law & Order. Doing this will probably be a major headache.

• Move all detectors in /detectors/
  • or at least all fibers into /fibers/

There are also several points where the code is "not good enough". This is, of course, debatable to some degree. Some problems will also require hard work to fix. Here are some points that always bothered me:

• Enable -Wall -Wextra -Wformat-security to get full information on coding problems from the compiler. Remember: The compiler is your best friend.
• Enable static code analysis to find more problems.
• Delete stale branches - no wait, delete every branch except dev, and make dev the new master/default.
• Delete stuff that is no longer needed. Maybe land/?
• Remove remains of Travis now that GitHub Actions is used for CI/CD.
• Remove superflous comments, e.g. /* * Default constructor. * Creates an instance of the task with default parameters. */
• Remove superflous constructors of this type: R3BSomething(const char* name, Int_t iVerbose = 1);
• Remove functions that don't do anything, e.g R3BSomething::FinishEvent() {}
• Check for nullptr where needed, especially in Init(), to catch wierd errors.
• Use types, e.g. TVector3 for postitions instead of indiviual x, y, z coordinates. Especially, use std::vector<TVector3> instead of double x[20]; double y[20]; double z[20]!
• Replace Fixed-Size C-Style arrays, especially where the input might not be fixed-size. This is a major reason for random crashes. Just use std::vector.
  Also, do not use char something[255]. Better string processing exists. To find these problems, start with searching for the regex \[\d+\].
• Restructure overly long Exec() functions: It is not necessary to cram everything into this single function - it is a good idea to refactor functions out into easily digestible parts. This makes your life so much easier. As a good starting point, search for goto.
• Replace Fortran components. There is no good reason to use Fortran in a C++ project. I have little domain knowledge on this topic, but a quick google search resulted in several Runge-Kutta solvers. Maybe we can adopt one of those?

[kresan]

Sounds like a good and reasonable plan. Let's go for it. Although it will take some time until we fill all points.

[hapol]

I'm still on vacations, and I had no time to review the suggestions. But I would like to thank @janmayer for the extensive list and all the work done!

[jose-luis-rs]

Thanks @janmayer.
Note that we already have git repositories to manage ucesb and upexps. In the R3B-Wiki we have some details about how to clone and update these repositories, see https://wiki.r3b-nustar.de/analysis/ucesb/overview

[janmayer]

Note that we already have git repositories to manage ucesb and upexps. In the R3B-Wiki we have some details about how to clone and update these repositories, see https://wiki.r3b-nustar.de/analysis/ucesb/overview

Well yes, the code is in git repositories. This means it is almost trivial to put them on GitHub and gain all the benefits.

[kresan]

@janmayer, I have moved all relevant documentation from r3broot.gsi.de to GitHub Wiki. If you are fine with it, I will remove the users from r3broot.gsi.de and write a ticket to IT with request to create a redirection to new location.

[klenze]

@janmayer I think I fully agree with those points.

I would put it more bluntly: Whenever an R3BRoot contributor copies+text-replaces a file, they are not productive. Instead, they are increasing the technological debt of the project, which is the opposite.

Classes/structs are supposed to describe data types. One class can have multiple instances. Let's say you have four different subsystems with white rabbit time stamps. How many classes/structs are appropriate? The answer is one, not four. You can have four separate instances, (or four TCAs, if you really want) however.

In a similar vein, the data read by the r3bsource code should not be separated by detector, but by readout system. So you have one class for handling califa-febex, creating one type of struct. It will work both for the califa and the PSP cases. Another class might handle any TAMEX data. (Obviously, califa and psps will process the data differently afterwards, but at the mapped stage, it is still essentially what was read out from the electronics modules.)

@inkdot7 maintains the ucesb git at https://git.chalmers.se/expsubphys/ucesb.git. I do not think we need github for the sake of github here. Håkan has to deal with the diffs people send him, so we can not really complain there.

The upexps repository is quite a different beast, however. Ideally, all the stuff independent of the current beam time would be in some separate top level directories, and the beamtime specific stuff would be just a few lines defining which detectors (using which lmd-ids) were used. The reality is far from that. The worst offender there is CALIFA, where the same parsing code gets copypasted for every single experiment.

Related to this is a thing called r3bmap, where the channel mappings and the like for the detectors are defined. At the moment, there is no explicit link between them, just the fact that the mapping_*.hh files in the experiment subfolders of upexps were probably at some point the output of some script residing in there. I do not know what the best way to make this link explicit would be. On the one hand, one could teach make how to auto-generate the relevant mapping files by calling the relevant scripts. This would also mean that for different experiments, we might have to have different mapping-generating scripts.

I also realise that both the upexps and the r3bmap are maintained by our DAQ (plus slow control plus network plus ...) wizards as a side project. From my understanding of their workload, I think it is unlikely that they will find the spare time to reorganise this stuff for the benefit of the R3BRoot users.

Regarding R3BRoot itself, the problem I see is that running clang-tidy & friends against our codebase and picking up the litter of sprintfs, uninitialized variables and the like carelessly dropped by uncaring contributors is a rather thankless and probably not very fulfilling task. Unless we are willing to pay someone explicitly to do that, it probably won't happen.

Also, I would propose that we add explicit code reviews of pull requests. And if someone e.g. tries to reinvent the format string "%02d" using an if statement, they will be told about the length options of format strings and kindly asked to use "%02d" instead. etc.

[inkdot7]

@klenze thanks for alerting me to this discussion.

Indeed, the UCESB git repository already have a gitlab, with quite some CI testing (example https://git.chalmers.se/expsubphys/ucesb/-/pipelines/36244 : 3 CPU architectures, 2 OSes, 2 compilers, 5 GNU/linux distributions - at least, and many versions of those). And - it runs some self-tests whenever is compiled. I happen to like the gitlab user interface, and its CI system. If someone wants to play around with a mirror on github - fine. (Not my problem.) The downside of all these automated tests is that I get almost no feedback from users any longer...

I'm sorry about the intricate innards of UCESB - those really are a problem. Whenever something needs to be implemented, there is a long effort - again and again - (for me) - to try to understand how it works. (I guess that means that on any average day, we have 0 persons knowing how UCESB gets the job done.) Suggestions (i.e. code) to simplify while achieving the same / similar effects, are always welcome... I try to remove some fissionable material whenever implementing something new, but in total it seems to stay too close to critical mass (http://www.catb.org/jargon/html/C/critical-mass.html) for comfort...

Why https://wiki.r3b-nustar.de/analysis/ucesb/overview is suggesting that users clone UCESB from the land account I do not know. The UCESB homepage http://fy.chalmers.se/~f96hajo/ucesb/ points to https://git.chalmers.se/expsubphys/ucesb. (Hoping here that someone will fix the wiki so I don't have to :-) )

As for UPEXPS, it is indeed infested with copy-paste. It has been put on https://git.gsi.de/r3b/upexps to try to address this. This allows to use and extend its gitlab CI, which however is in disrepair due to not being used for a long time... That repository is not marked public since it needs to be reviewed for release before that. I.e. currently requires a GSI web login. Some volunteers for the general cleanup have shown up, but that effort got distracted by vacation-times.