Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

Fix warnings that will be treated as errors in CMSSW #367

Merged
merged 8 commits into from
Feb 5, 2024
Merged

Conversation

ariostas
Copy link
Member

I fixed a bunch of warnings that are treated as errors in CMSSW by default. The main thing was fixing comparisons between signed and unsigned integers. I changed some variable to be unsigned integers, since it makes more sense for them to be unsigned. I also had to remove a bunch of unused variables. I was undecided between deleting the lines or commenting them out. I decided on deleting them since they looked like remnants from old code.

I also removed the dependency on cppitertools, which was a very minor change that I don't think necessitates its own PR.

There are still a lot of warnings remaining. I think it is definitely worth taking a look at them since some of them look important. However, this PR is already somewhat lengthy, so maybe I'll do that in a later PR.

Also, I noticed that there is a lot of repeated code. I think that it would be worth doing some refactoring at some point to make things more modular.

@ariostas
Copy link
Member Author

/run standalone

Copy link

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

int* nMDs; //counter per module
int* totOccupancyMDs; //counter per module
unsigned int* nMDs; //counter per module
unsigned int* totOccupancyMDs; //counter per module
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking if it's more practical to switch to explicit size integer types.
like int32_t and uint32_t

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking if it's more practical to switch to explicit size integer types.

this is outside of this PR scope

Copy link
Member Author

Choose a reason for hiding this comment

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

that's true, I'll think about how much work that would be

@slava77
Copy link
Contributor

slava77 commented Feb 2, 2024

@ariostas
when you are done, please run a (standalone) test

@ariostas
Copy link
Member Author

ariostas commented Feb 5, 2024

/run standalone

Copy link

github-actions bot commented Feb 5, 2024

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

The full set of validation and comparison plots can be found here.

@slava77 slava77 merged commit 58a290c into master Feb 5, 2024
2 checks passed
@ariostas ariostas deleted the werrors_cmssw branch February 23, 2024 21:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants