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

Consolidate tools handling noise #98

Merged
merged 13 commits into from
Jul 25, 2024

Conversation

giovannimarchiori
Copy link
Contributor

@giovannimarchiori giovannimarchiori commented Jul 19, 2024

Consolidate code of tools handling noise, improve readability, remove duplications, add tool to add noise to cells for fcc-ee.
Needs key4hep/k4FWCore#214

@giovannimarchiori giovannimarchiori marked this pull request as draft July 19, 2024 16:50
@giovannimarchiori giovannimarchiori marked this pull request as ready for review July 22, 2024 09:45
giovannimarchiori and others added 2 commits July 22, 2024 14:15
…stNoiseTool and set it to false by default to avoid having to specify the list of offsets in python
Copy link
Contributor

@BrieucF BrieucF left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@BrieucF
Copy link
Contributor

BrieucF commented Jul 25, 2024

@giovannimarchiori
Copy link
Contributor Author

@@ -44,7 +44,7 @@ StatusCode TopoCaloNoisyCells::initialize() {
double readNoisyCells;
double readNoisyCellsOffset;
tree->SetBranchAddress("cellId", &readCellId);
tree->SetBranchAddress("noiseLevel", &readNoisyCells);
tree->SetBranchAddress("noiseLevel", &readNoisyCells); // would be better to call branch noiseRMS rather than noiseLevel
Copy link
Contributor

Choose a reason for hiding this comment

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

What is blocking to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, this would mean that the noise map files have to be regenerated changing name to the rms branch. I would be ok with that but don’t want to disrupt the user who have already noise files at hand. What do you and @BrieucF prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can handle this in another PR

@BrieucF BrieucF merged commit 05a64eb into HEP-FCC:main Jul 25, 2024
0 of 5 checks passed
@jmcarcell jmcarcell mentioned this pull request Jul 29, 2024
@giovannimarchiori giovannimarchiori deleted the gmarchio-main-20240719 branch August 29, 2024 08:17
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.

3 participants