Skip to content

[hist] function to get sum of weights including under/overflows#18232

Merged
hageboeck merged 6 commits intoroot-project:masterfrom
ferdymercury:sumweights
Apr 4, 2025
Merged

[hist] function to get sum of weights including under/overflows#18232
hageboeck merged 6 commits intoroot-project:masterfrom
ferdymercury:sumweights

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

@ferdymercury ferdymercury commented Apr 2, 2025

This Pull request:

Changes or fixes:

Fixes #8951

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury requested a review from lmoneta as a code owner April 2, 2025 10:24
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2025 •
edited
Loading

Test Results

    19 files      19 suites   5d 4h 17m 43s ⏱️
 2 717 tests  2 713 ✅ 0 💤 4 ❌
49 832 runs  49 827 ✅ 1 💤 4 ❌

For more details on these failures, see this check.

Results for commit 3ea90b0.

♻️ This comment has been updated with latest results.

hageboeck
hageboeck reviewed Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@hageboeck hageboeck left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello @ferdymercury,

I would try to copy as few code as possible, so I find the suggestion of the person that opened the issue quite reasonable:
GetSumOfWeights(bool includeOverflow = false)

The function continues to work, and internally, you change:

- Int_t binx,biny,binz;
- for(binz=1; binz<=fZaxis.GetNbins(); binz++) {
+ const auto zMax = overflow ? fZaxis.GetNbins()+2 : fZaxis.GetNbins()+1;
+ for(Int_t binz = overflow ? 0 : 1; binz < zMax; binz++) {

This would even reduce the calls to GetNbins() during the loops.
What do you think?

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Apr 3, 2025 •
edited
Loading

Thanks for the review!
Yep, I agree with you and I would have done that in the beginning, but @guitargeek made me aware that it's not a good idea to change signature of virtual functions as it may break downstream user code, as it happened recently with GetExpFormula. That's why I went towards a new function.
How shall I proceed?
Should I maybe move the implementation to the new function and redirect the old one to call the new one?

@ferdymercury
[hist] avoid code duplication
e967dc2 
as suggested by stephan
@ferdymercury ferdymercury requested a review from hageboeck April 3, 2025 09:44
@hageboeck
Copy link
Copy Markdown
Member

hageboeck commented Apr 3, 2025

Should I maybe move the implementation to the new function and redirect the old one to call the new one?

Yes, that's a good approach! But don't make the implementation virtual, so the compiler can do the best job. You could call it GetSumOfAllWeights(bool includeOverflow), and then direct the original implementation to use that. Now that the original implementation becomes trivial, you can move it to the header.

ferdymercury
ferdymercury commented Apr 3, 2025
View reviewed changes
ferdymercury
ferdymercury commented Apr 3, 2025
View reviewed changes
hageboeck
hageboeck reviewed Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

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

Less code, less variables. What do you think?

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Apr 3, 2025

Less code, less variables. What do you think?

Agreed and pushed, thanks for the idea ! :)

hageboeck
hageboeck reviewed Apr 3, 2025
View reviewed changes
hageboeck
hageboeck approved these changes Apr 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! Let's see what the CI says.

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Apr 3, 2025

Thanks, LGTM! Let's see what the CI says.

Thanks to you! Looking green.

@hageboeck hageboeck merged commit 4a04f51 into root-project:master Apr 4, 2025
19 of 23 checks passed
@ferdymercury ferdymercury deleted the sumweights branch April 4, 2025 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hageboeck hageboeck hageboeck approved these changes

@lmoneta lmoneta Awaiting requested review from lmoneta

Assignees

@lmoneta lmoneta

@hageboeck hageboeck

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add optional flag to include underflow and overflow weights when returning sum of weights from histogram

3 participants

@ferdymercury @hageboeck @lmoneta