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

Iss1172 - Memory bugs and typos in DQM code #1219

Merged
merged 16 commits into from
Oct 31, 2023
Merged

Iss1172 - Memory bugs and typos in DQM code #1219

merged 16 commits into from
Oct 31, 2023

Conversation

EinarElen
Copy link
Contributor

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

Hint: Use the phrase 'This resolves #< issue number >' so that they are linked automatically.
This resolves #1172 and one of the entries in #1166 (missing & in PN DQM).

I expect this to fail the validation step for the Hcal but not for the PN DQM, we have changed the histograms in the HcalDQM.

The following warning/static-analysis/cleanup was also done

  • Add missing virtual destructors
  • override final -> override
  • Remove tautological check (pdgCode < 10000000000) )
  • Fallthrough and default cases in switches
  • Removing unused variables

@EinarElen EinarElen changed the title Iss1172 Iss1172 - Memory bugs and typos in DQM code Oct 31, 2023
Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

This looks good but I am curious about where histogram content is being changed - I don't see anything affecting fill or binning calls outside of white-space formatting.

@EinarElen
Copy link
Contributor Author

This bugfix (it's subtle, thanks implicit conversion warnings!)

9305a03

@EinarElen
Copy link
Contributor Author

The plots that are failing are the expected ones. The Hcal one is not failing because that is the only one where we arent running the particular analyzer :)

@tomeichlersmith
Copy link
Member

tomeichlersmith commented Oct 31, 2023

Since only one plot fails for each of the three failing validation tests, I am posting it here.1 @EinarElen do these make sense? It seems like a physical change (i.e. I don't see any screaming bugs), but I'm unsure how to interpret the change.

Signal

signal-HcalInefficiencyAnalyzer_HcalInefficiencyAnalyzer_efficiency

Inclusive

inclusive-HcalInefficiencyAnalyzer_HcalInefficiencyAnalyzer_efficiency

Ecal PN

ecal_pn-HcalInefficiencyAnalyzer_HcalInefficiencyAnalyzer_efficiency

Edit: Cropped PNGs so there's less ugly white-space.

Footnotes

  1. I got this plot by downloading the artifacts for the failing PR Validation jobs. Unzipping and unpacking and then copying out the plot in the "fail" subdirectory of the unpacked directory. I then used pdftoppm to convert the PDF plots to PNG so GitHub will render them.

@EinarElen
Copy link
Contributor Author

It is a physical change in that the previous version was just straight up invalid. What the analyzer is trying to do is to check if any hit has been above threshold in the different hcal sections and if so, what was the earliest layer that had such a hit. By default we have each section set to a negative "failed to veto" value. The histogram is supposed to contain different categories like "the hit was vetoed by one of the side hcal modules" or "the hit was only vetoed by the back hcal"


  bool vetoedByBack{firstLayersHit[ldmx::HcalID::HcalSection::BACK] !=
                    failedVeto};
  bool vetoedByTop{firstLayersHit[ldmx::HcalID::HcalSection::TOP]};
  bool vetoedByBottom{firstLayersHit[ldmx::HcalID::HcalSection::BOTTOM]};
  bool vetoedByRight{firstLayersHit[ldmx::HcalID::HcalSection::RIGHT]};
  bool vetoedByLeft{firstLayersHit[ldmx::HcalID::HcalSection::LEFT]};
  bool vetoedBySide{vetoedByTop || vetoedByBottom || vetoedByRight ||
                    vetoedByLeft};

Problem is right here, only the first (vetoedByBack) condition is correct, all the others have missed the != failedVeto part.

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

Thanks for the extra explanation! This looks good and I can make a tag release after merging.

@tomeichlersmith tomeichlersmith merged commit 7bd260e into trunk Oct 31, 2023
4 of 7 checks passed
@tomeichlersmith tomeichlersmith deleted the iss1172 branch October 31, 2023 21:07
@tvami tvami mentioned this pull request Sep 2, 2024
7 tasks
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.

Bug and typo in HcalDQM
2 participants