-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Implement TH2Poly in DQM Services for HGCal DQM #41932
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
Merged
Merged
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
8041853
Implement TH2Poly in DQM Services for HGCal DQM
ywkao 419cb6f
Apply changes from code quality checks
ywkao 5284c46
Add a patch based on makortel's comments
ywkao fb37861
Implement TH2Poly support in DQMServices
ywkao 7d85734
Extend DQMServices testing for TH2Poly
ywkao f750a33
Apply code-format patch
ywkao cca126b
Fix IndexError in the DQM unit test for TH2Poly
ywkao eb41648
Fix TypeError in `DQMServices/Demo/test/dqmiodumpentries.py`. Omit De…
ywkao 59617bc
Fix typo in DataFormats/Histograms/src/classes_def.xml
ywkao 07470c1
apply code checks and format recommendation
ywkao c0af007
Fix TH2Poly handling in DQM Monitor Elements
ywkao 7755770
Fix MonitorElement::getNcells() dimension handling
ywkao 4fed83b
Remove commented TH2Poly code as requested by reviewers
ywkao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ | |
"TH2Fs", | ||
"TH2Ss", | ||
"TH2Ds", | ||
"TH2Polys", | ||
"TH3Fs", | ||
"TProfiles", | ||
"TProfile2Ds", | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this code? Is something needed here? (I have no idea what exactly is being done in this function in general)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure all the details either. I referred to PR#37665-files, the implementation of DQM support of TH1I and TH2I.
@mmusich could you help evaluate the necessity of having TH2Poly::GetArray() functionality? Maybe we need help from ROOT experts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the question on this commented out code is still open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @makortel, I'm addressing the open question about the commented-out TH2Poly code. After examining the issue, I have found that while TH2Poly lacks a
GetArray()
method, we can create a simple workaround:and the DQM code can be:
Since the system already correctly handles the lack of TH2Poly support through the case/default structure, the currently commented-out code doesn't affect system functionality.
Would you prefer I implement this in the current PR, or would it be better to address it in a separate, focused PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is up to @cms-sw/dqm-l2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advise removing the commented-out code and address the introduction of a
GetArray
method for TH2poly in a separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I will follow the guidance.