Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement particle filtering for neuland simulation #1083

Merged
merged 1 commit into from
Dec 1, 2024

Conversation

SimonL2718
Copy link

New features:

Allow filtering for certain particles during digitization. Setup is done within neulandAna.cxx executable.


Checklist:

Sorry, something went wrong.

@YanzhaoW
Copy link
Contributor

Hi @jose-luis-rs . @SimonL2718 is a bachelor student from my group. Could you add him to the contributors such that the CI workflow could run?

Copy link
Contributor

@YanzhaoW YanzhaoW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, Simon

I leave some comments. Please fix them.

CMakeLists.txt Outdated
@@ -549,6 +549,7 @@ if(NOT MODULE)
add_subdirectory(tracking)
add_subdirectory(passive)
add_subdirectory(tcal)
add_subdirectory(r3bgen)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe don't change it here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be with the entries for r3bbase and r3bdata or somewhere else entirely?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, just revert back to the dev version. (between music and lumon).

void SetFilter(R3B::Neuland::BitSetParticle filtered_particles, double minimum_allowed_energy);
[[nodiscard]] auto GetFilter() const -> R3B::Neuland::BitSetParticle { return filtered_particles_; }
[[nodiscard]] auto GetMinimumAllowedEnergy() const -> double { return minimum_allowed_energy_; }
auto ShouldNeulandPointBeFiltered(const R3BNeulandPoint& neuland_point) -> bool;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name is too long. Your input parameter already contains NeulandPoint type and you don't need to mention it again in the function name.

Suggested change
auto ShouldNeulandPointBeFiltered(const R3BNeulandPoint& neuland_point) -> bool;
auto CheckFiltered(const R3BNeulandPoint& neuland_point) -> bool;

Comment on lines 94 to 95
// energy in GeV
double minimum_allowed_energy_ = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the comments in the same line.

Suggested change
// energy in GeV
double minimum_allowed_energy_ = 0;
double minimum_allowed_energy_ = 0; // energy in GeV

Comment on lines 25 to 28
auto const neutron_PID = 2112;
auto const Sn_p = int{ 50 };
auto const Sn_z = int{ 123 };
auto const BeamEnergyAtTarget = 883.;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto const neutron_PID = 2112;
auto const Sn_p = int{ 50 };
auto const Sn_z = int{ 123 };
auto const BeamEnergyAtTarget = 883.;
constexpr auto neutron_PID = 2112;
constexpr auto Sn_p = 50;
constexpr auto Sn_z = 123;
constexpr auto BeamEnergyAtTarget = 883.;

Comment on lines 29 to 32
constexpr auto seconds_to_nanoseconds = 1e9;
static constexpr double BirkdP = 1.032;
static constexpr double BirkC1 = 0.013 / BirkdP;
static constexpr double BirkC2 = 9.6e-6 / (BirkdP * BirkdP);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constexpr auto seconds_to_nanoseconds = 1e9;
static constexpr double BirkdP = 1.032;
static constexpr double BirkC1 = 0.013 / BirkdP;
static constexpr double BirkC2 = 9.6e-6 / (BirkdP * BirkdP);
constexpr auto seconds_to_nanoseconds = 1e9;
constexpr auto BirkdP = 1.032;
constexpr auto BirkC1 = 0.013 / BirkdP;
constexpr auto BirkC2 = 9.6e-6 / (BirkdP * BirkdP);

{
// Apply Birk's law ( Adapted from G3BIRK/Geant3)
if (charge != 0 && length > 0)
{
Double_t birkC1Mod = BirkC1;
double birkC1Mod = BirkC1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
double birkC1Mod = BirkC1;
constexpr auto birkC1Mod = BirkC1;

Comment on lines 56 to 59
: FairMCPoint(point)
, fLightYield(lightYield)
, particle_id_(particle_id)
, parent_particle_id_(parent_particle_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use curly brackets:

Suggested change
: FairMCPoint(point)
, fLightYield(lightYield)
, particle_id_(particle_id)
, parent_particle_id_(parent_particle_id)
: FairMCPoint { point }
, fLightYield{ lightYield }
, particle_id_{ particle_id }
, parent_particle_id_{ parent_particle_id}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change this for other construction methods of NeulandPoints aswell or just this one in particular?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, better change the other construction methods of neuland points.

Copy link
Contributor

@YanzhaoW YanzhaoW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some tips to fix the warnings from Clang-tidy

Comment on lines 42 to 44
const Double_t lightYield,
const int particle_id,
const int parent_particle_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just tell static analyser to ignore this warning by adding NOLINTBEGIN and NOLINTEND

// NOLINTBEGIN
R3BNeulandPoint(const Int_t trackID,
                    const Int_t detID,
                    const TVector3& pos,
                    const TVector3& mom,
                    const Double_t tof,
                    const Double_t length,
                    const Double_t eLoss,
                    const UInt_t EventId,
                    const Double_t lightYield,
                    const int particle_id,
                    const int parent_particle_id)
// NOLINTEND

constexpr auto neutron_pid = 2112;
constexpr auto eLoss_proton = 0.7;
constexpr auto eLoss_neutron = 0.3;
TVector3 sample_vector;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TVector3 sample_vector;
const auto sample_vector = TVector3{};

NOTE: you should always use following style to initialize a variable:

auto var_name = Type {}; // const auto if necessary


NeulandPointFilter filter;

R3BNeulandPoint protonPoint(0, 0, sample_vector, sample_vector, 0., 0., eLoss_proton, 0, 0., proton_pid, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
R3BNeulandPoint protonPoint(0, 0, sample_vector, sample_vector, 0., 0., eLoss_proton, 0, 0., proton_pid, 0);
const auto protonPoint = R3BNeulandPoint {0, 0, sample_vector, sample_vector, 0., 0., eLoss_proton, 0, 0., proton_pid, 0};

NeulandPointFilter filter;

R3BNeulandPoint protonPoint(0, 0, sample_vector, sample_vector, 0., 0., eLoss_proton, 0, 0., proton_pid, 0);
R3BNeulandPoint neutronPoint(0, 0, sample_vector, sample_vector, 0., 0., eLoss_neutron, 0, 0., neutron_pid, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
R3BNeulandPoint neutronPoint(0, 0, sample_vector, sample_vector, 0., 0., eLoss_neutron, 0, 0., neutron_pid, 0);
const auto neutronPoint = R3BNeulandPoint {0, 0, sample_vector, sample_vector, 0., 0., eLoss_neutron, 0, 0., neutron_pid, 0};

@YanzhaoW
Copy link
Contributor

YanzhaoW commented Nov 1, 2024

To fix the clang-format issue, just click its "Details", find the file names, go there and use "Space f m" to autoformat the file in neovim.

Comment on lines 26 to 27
R3BNeulandPoint()
: FairMCPoint{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
R3BNeulandPoint()
: FairMCPoint{}
R3BNeulandPoint()

@@ -42,21 +42,21 @@ class R3BNeulandPoint : public FairMCPoint
const Double_t lightYield,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just disable this warning by:

// NOLINTBEGIN
R3BNeulandPoint(const Int_t trackID,
                    const Int_t detID,
                    const TVector3& pos,
                    const TVector3& mom,
                    const Double_t tof,
                    const Double_t length,
                    const Double_t eLoss,
                    const UInt_t EventId,
                    const int particle_id,
                    const int parent_particle_id)
// NOLINTEND

@YanzhaoW YanzhaoW mentioned this pull request Nov 28, 2024
4 tasks
@jose-luis-rs
Copy link
Contributor

Hi, this branch was squashed in #1084

@jose-luis-rs jose-luis-rs merged commit e34265b into R3BRootGroup:dev Dec 1, 2024
6 checks passed
@jose-luis-rs
Copy link
Contributor

Please @SimonL2718 reopen this PR. It should be approved by the NeuLAND WG before merging.

@YanzhaoW
Copy link
Contributor

YanzhaoW commented Dec 3, 2024

Please see this #1086 for the further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants