Possible contribution: how to proceed?

Gitlab Discussions
,
1

Greetings,

I recently decided to try to contribute to the CASToR software since the developer took the time to provide us such an easy way to do so. However, I must admit I am not sure how to proceed when it is not for a bug. As such, here I am!

Here is my first attempt: add to castor-GATERootToCastor the capability to enforce radial and axial restrictions to the data being converted.

Features:

  • A flag to enforce radial and axial restrictions as described in the geom file.
  • Add to the Cdh file created the axial restriction described in the geom file.
    • I still need to validate that it is still needed since the radial restriction is read by castor-recon just fine from the geom.
  • List-mode and Histo-mode datasets can now be smaller! (Only really worth for Histo-mode, most likely)
  • A counter for the events removed.

Limitations:

  • Only applied to PET (I did not take the time to look at the other type of modality)
  • File created are less traceable. Nothing in the header of the file knows that this filtering was applied.
  • There are most likely some more that I have not thought…

I recently created a version based on the current version of the master branch and it work like my initial version (based on CASToR v2.1.1) but I still have 1-2 sanity checks to do. I also created a version based on the current version of the develop branch but I still haven’t tested it.

So, from there, I do not know what to do with it. Should I simply push the branch (If so, which one?)? Wait for a validation of the relevance of that feature? Is there a minimum requirement before push-ing a potential feature?

Bests,
Maxime Toussaint

P.S.: I have seen some features that are coming by playing in the develop branch and I must say I am thrilled! Keep up the good work!

P.S.S: In castor-GATERootToCastor.cc, nCrystalsTot is a unint32_t which means that the following line:

uint64_t nb_bins = (nCrystalsTot*nCrystalsTot - nCrystalsTot)/2;

can results in inccorrect values. The following modification worked for me:

uint64_t nb_bins = ((uint64_t)nCrystalsTot*(uint64_t)nCrystalsTot - (uint64_t)nCrystalsTot)/2;

2

Hi Maxime,

First thank for your contribution!

Very good question. I moved your message to the Gitlab discussions section which is dedicated to this purpose: let the users present their potential contribution they would like to share, exactly as you did.

The idea is that once you have something ready:

  • If not already done, create a new branch with either the prefix feature/ (for new functionalities, e.g feature/new_projector), or hotfix/ (for bug correction).
  • Then push it to the public gitlab server.
  • From there you (or us) can create a merge request into develop for us to review. We avoid to merge directly into master, instead merge new content into develop and then to master.

Some remarks:

  • It is not mandatory to create a thread here to describe the new branch, but we believe it is more convenient to do so. At least any new branch should have a small branch_description.txt text file which overviews the new content and purpose of the branch.
  • Not any contribution has the objective to be merged into develop as some features may be complicated to merge with other features. They can perfectly remained as feature/ branch in the Gitlab and still be useful for the community.
  • This section is more dedicated to present a new branch or discuss compatibilities and purposes. For more technical discussions (test result, bug notification, etc…), it is better to discuss directly in Gitlab (issue/comments) than here in order to avoid mail spam for members who enabled mailing-list mode.

Let me know if you have any question or remarks.

PS: Just for information, there is currently a WiP branch dedicated to various upgrades to castor-GATERootToCastor. It changes the main code quite a lot in order to allow multi-threaded processing of ROOT files.

Thanks for both your P.Ss remarks!

Best,
Thibaut

3

Thank you for all that information!

Another question: do you have a preference over the origin of a feature being submitted? I guess you would welcome any contribution, as such, you do not want to enforce a base. However, I already have a version based on the master branch and one based on “yesterday” develop branch. While I have mostly finished my testing of the former, the latter is currently not tested. Since you have refactored greatly castor-GATERootToCastor in the develop branch, I would think you prefer the latter, but I just want to make sure.

4

Yes the develop branch is preferred as a origin as it gathers potential development from other feature branches. Besides it also contains a test pipeline so that we can checked if something is wrong before merging.

And indeed I had to rework a bit the converter main code before considering multithreading. I hope it will not cause you extra-work though!

5

I just push-ed the branch to gitlab. I tested with extreme cases of radial and axial restriction and it seems to work as intended.

The goal/feature/limitation are written in the file branch_description.txt, if need be.

A part that I am unsure is how I deal with event/lor that were removed by the axial/radial restriction. Currently, they are considered as a new kind (removed) which subtract to the total number of lor. Because of that choice, I needed to modify the input of the method ProcessPETEvent and the method IsAvailableLOR is called two times.

Also, while doing this, I stumbled upon two things:

  1. The line layer = layerID; in src/management/gDataConversionUtilities.cc started to create problem even after I changed layer to uint32_t. Now, I use int32_t which seems to work…
  2. Why is Out_data_file->SetNbEvents called two times, with only one instruction in between? (See occurence 4th and 5th of Out_data_file->SetNbEvents)

I might create Issue in the gitlab for those two, if I have the time tomorrow.

If you need anything, do not hesitate!

6

Hello Maxime,

Just to let you know that, in parallel to you, I also implemented a radial restriction on the LoR. I needed it for castor-proj, a simulation tool based on CASToR. The modifications were implemented in the branch feature/castor-proj-asim.

I addition to m_maxAxialDiffmm, I created m_maxRadialLORmm and used it in iScannerPET::IsAvailableLOR(). The following classes were modified:

  • sScannerManager

  • iScannerPET

  • vScanner

  • iDateFilePET

Best,

7

Thank you for letting me know!

Since feature/RootConv_applyRadialAxialRestriction basically only call already implemented restriction, it is highly probable that the only change that will need to be done when feature/castor-proj-asim will be merged is how the call are made. A quick look at the said branch and it seems it might require no change, except to read the new restriction.