FairRootGroup / FairRoot

C++ simulation, reconstruction and analysis framework for particle physics experiments
http://fairroot.gsi.de
Other
57 stars 96 forks source link

Allow for 3D box in FairBoxGenerator #1576

Closed cvilelahep closed 4 weeks ago

cvilelahep commented 1 month ago

The current FairBoxGenerator allows only for generating particles over a plane as the depth of the box, dZ, is hard-coded to be 0.

In this pull request, the SetBoxXYZ method is overloaded to accept also two values of z (z1 and z2) from which dZ is computed in the same way as dX and dY.


Checklist:

coderabbitai[bot] commented 1 month ago
Walkthrough ## Walkthrough The `FairBoxGenerator` class has been improved by updating the `SetBoxXYZ` method to include additional parameters, `z1` and `z2`, allowing for precise definitions of box dimensions in the Z-axis. This enhancement increases flexibility while ensuring backward compatibility. Additionally, the contributors list now acknowledges Cristovao Vilela's contributions, complete with an ORCID identifier for better tracking. ## Changes | File | Change Summary | |------------------------------------------------|----------------| | `fairroot/generators/FairBoxGenerator.cxx` | Enhanced `SetXYZ` for better readability; modified `SetBoxXYZ` to include additional parameters `z1` and `z2`, with updated internal logic for Z-dimension calculations. | | `fairroot/generators/FairBoxGenerator.h` | Updated `SetBoxXYZ` method declaration to accept `z1` and `z2`, improving flexibility while retaining the original method for backward compatibility. | | `CONTRIBUTORS` | Added Cristovao Vilela to the list of contributors, including an ORCID identifier. | | `.zenodo.json` | Added new entry for Cristovao Vilela with an ORCID identifier to the metadata structure. |

Recent review details **Configuration used: CodeRabbit UI** **Review profile: CHILL**
Commits Files that changed from the base of the PR and between 67ac4969541f8d71f784bd32f17462632a7b5684 and 08726b4b590d876382d013e2e10c7cee62f2c674.
Files ignored due to path filters (1) * `codemeta.json` is excluded by `!**/*.json`
Files selected for processing (2) * .zenodo.json (1 hunks) * CONTRIBUTORS (1 hunks)
Files skipped from review due to trivial changes (1) * CONTRIBUTORS
Additional comments not posted (1)
.zenodo.json (1)
`165-169`: **Entry for Cristovao Vilela is correctly formatted and consistent.** The new entry for Cristovao Vilela follows the established structure and includes the necessary fields.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
karabowi commented 1 month ago

@cvilelahep , can you please apply clang-format to the files? Can you also update the license headers in both files by changing second line to: * Copyright (C) 2014-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH *?

cvilelahep commented 1 month ago

@cvilelahep , can you please apply clang-format to the files? Can you also update the license headers in both files by changing second line to: * Copyright (C) 2014-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH *?

@karabowi done.

karabowi commented 1 month ago

There is a problem with your function. It is in conflict with the old function SetXYZ():

/Users/alfaci/alfaci/workspace/FairRootGroup_FairRoot_PR-1576/examples/simulation/Tutorial4/macros/run_tutorial4.C:107:13: error: call to member function 'SetBoxXYZ' is ambiguous
    boxGen->SetBoxXYZ(-20., -20., 20., 20., 0.);

Maybe remove default values?

cvilelahep commented 1 month ago

There is a problem with your function. It is in conflict with the old function SetXYZ():

/Users/alfaci/alfaci/workspace/FairRootGroup_FairRoot_PR-1576/examples/simulation/Tutorial4/macros/run_tutorial4.C:107:13: error: call to member function 'SetBoxXYZ' is ambiguous
    boxGen->SetBoxXYZ(-20., -20., 20., 20., 0.);

Maybe remove default values?

Ah, of course, sorry for missing this. I removed the default values.

karabowi commented 1 month ago

@cvilelahep , sine your changes you should apply clang-format again. Also, you should squash your commits into one and write a nice commit message following our guidelines like G3 and G4. Please also add your name to the CONTRIBUTORS file, in a separate commit.

karabowi commented 4 weeks ago

we maybe should update one example with the new feature (and keep another with the old one)?

@ChristianTackeGSI good idea, I have created an issue not to forget.