Skip to content

PR for ALLEGRO digitization updates #173

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.

Sign up for GitHub

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

Jump to bottom
Open
SSinngh99 wants to merge 35 commits into
base: main
Choose a base branch
from
Open

PR for ALLEGRO digitization updates #173

SSinngh99 wants to merge 35 commits into from

Conversation

@SSinngh99
Copy link

@SSinngh99 SSinngh99 commented Sep 15, 2025

BEGINRELEASENOTES

  • Added digitization algorithms based on ATLAS LAr calorimeter
  • Added a preliminary algorithm to add noise to digits
  • Added algorithms to compute the whitening and matched filters
  • Added steering file as an example

ENDRELEASENOTES

giovannimarchiori
giovannimarchiori reviewed Sep 16, 2025
View reviewed changes
giovannimarchiori
giovannimarchiori reviewed Sep 16, 2025
View reviewed changes
giovannimarchiori
giovannimarchiori reviewed Sep 16, 2025
View reviewed changes
giovannimarchiori
giovannimarchiori reviewed Sep 16, 2025
View reviewed changes
@giovannimarchiori
Copy link
Contributor

giovannimarchiori commented Oct 14, 2025

Hello @SSinngh99 did you have time to look at the comments from my review? Also, would you please update the branch?
Thanks!

giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
RecFCCeeCalorimeter/src/components/CaloDigitizer.cpp Outdated
@@ -0,0 +1,155 @@
/*
* Copyright (c) 2014-2024 Key4hep-Project.
Copy link
Contributor

@giovannimarchiori giovannimarchiori Nov 24, 2025

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2014-2024 Key4hep-Project.
* Copyright (c) 2014-2025 Key4hep-Project.

giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
RecFCCeeCalorimeter/src/components/CaloFilter.cpp Outdated
@@ -0,0 +1,276 @@
/*
* Copyright (c) 2014-2024 Key4hep-Project.
Copy link
Contributor

@giovannimarchiori giovannimarchiori Nov 24, 2025

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2014-2024 Key4hep-Project.
* Copyright (c) 2014-2025 Key4hep-Project.

giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
RecFCCeeCalorimeter/src/components/CaloWhiteningFilter.cpp Outdated

for (unsigned int i = 0; i < WhitenedPulse.size(); i++) {
WhitenedDigit.addToAmplitude(WhitenedPulse[i]);
// std::cout << "WhitenedPulse[" << i << "] = " << WhitenedPulse[i] << std::endl;
Copy link
Contributor

@giovannimarchiori giovannimarchiori Nov 24, 2025

Choose a reason for hiding this comment

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

remove, or use debug() << ...

Copy link
Author

@SSinngh99 SSinngh99 Dec 1, 2025

Choose a reason for hiding this comment

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

Done.

giovannimarchiori
giovannimarchiori reviewed Nov 24, 2025
View reviewed changes
RecFCCeeCalorimeter/src/components/CaloWhiteningFilter.cpp Outdated
Comment on lines 178 to 179
// std::cout << "Digits w/ noise, no mu subtraction[" << i << "] = " << Digits[i] << std::endl;
// std::cout << "Pulse after Mu subtraction[" << i << "] = " << Pulse[i] << std::endl;
Copy link
Contributor

@giovannimarchiori giovannimarchiori Nov 24, 2025

Choose a reason for hiding this comment

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

remove, or use debug() << ..

Copy link
Author

@SSinngh99 SSinngh99 Dec 1, 2025

Choose a reason for hiding this comment

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

Done.

@giovannimarchiori
Copy link
Contributor

giovannimarchiori commented Nov 24, 2025

Hi @SSinngh99
I added a few further small review comments.
Also, please add a (short) unit test of your tool to https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/CMakeLists.txt
Thanks!
Giovanni

@SSinngh99
Copy link
Author

SSinngh99 commented Dec 1, 2025

Hi @giovannimarchiori,
I added the unit test as you suggested and addressed the comments. Please let me know if there is any additional modification that needs to be made for the unit test.
Best Regards,
Sahibjeet Singh

@BrieucF
Copy link
Contributor

BrieucF commented Dec 1, 2025

Hi Sahibjeet, thanks for this PR. Could you please add a Doxygen like comment to the four new transformers introduced here? Something like this https://github.com/key4hep/k4RecTracker/blob/master/DCHdigi/include/DCHdigi_v02.h#L1-L55

@SSinngh99
Copy link
Author

SSinngh99 commented Dec 2, 2025

Dear @BrieucF,

I just pushed the doxygen like documentation for the transformers and all the relevant functions.

Best Regards,
Sahibjeet Singh

@SSinngh99
Copy link
Author

SSinngh99 commented Dec 2, 2025

Dear @BrieucF,

I just pushed a few minor changes that fix the compilation warning. That seems to be why the build was failing.

Best Regards,
Sahibjeet Singh

@BrieucF
Copy link
Contributor

BrieucF commented Dec 4, 2025 •
edited
Loading

Hi Sahibjeet, the new test is failing: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/19864787090/job/56968490717?pr=173#step:4:665

From a quick look, the problem is likely that the name of the input file is wrong in RecFCCeeCalorimeter/tests/options/runDigitizationAndMatchedFilter.py, it should be ALLEGRO_sim_ee_z_qq.root. It may also need to access NoiseInfoTest_New_Modded4InvCorr? If yes, you can try with XROOTD (see e.g. 948d7af), or we place it in a web page where it can be retrieved with CMAKE commands or wget.

@SSinngh99
Copy link
Author

SSinngh99 commented Dec 12, 2025

Hi @BrieucF,

I tried using ALLEGRO_sim_ee_z_qq.root and ALLEGRO_sim_e.root (found in one of the other test files) but both of them seem to crash. It seems to me that the test is trying to find these files in the nominal git repo but cannot find anything there.

I am not sure if I need to make the file myself or not....

As for the NoiseInfoTest_New_Modded4InvCorr, that is a file which is produced as part of the AddNoise algorithm so we are okay on that front.

Best Regards,
Sahibjeet Singh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giovannimarchiori giovannimarchiori giovannimarchiori left review comments

At least 0 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SSinngh99 @giovannimarchiori @BrieucF @sasingh99