Skip to content

Conversation

@couet
Copy link
Member

@couet couet commented Jun 5, 2024

As said in this old forum post https://root-forum.cern.ch/t/thstack-th2-with-negative-values/16161/5, stacking a histogram with negative bin content does not make sense. This issue is not relevant #15740. But it is goo to make it clear in the documentation and to prevent adding such histograms in a stack. That's what this PR is doing.

@couet couet requested a review from linev June 5, 2024 14:12
@couet couet self-assigned this Jun 5, 2024
@couet couet requested a review from lmoneta as a code owner June 5, 2024 14:12
@linev
Copy link
Member

linev commented Jun 5, 2024

Therefore, histograms with a negative minimum will be rejected.

How it is done? Currently I see no protection for negative bins content.

@github-actions
Copy link

github-actions bot commented Jun 5, 2024

Test Results

    11 files      11 suites   2d 12h 14m 46s ⏱️
 2 645 tests  2 645 ✅ 0 💤 0 ❌
27 455 runs  27 455 ✅ 0 💤 0 ❌

Results for commit a76d35a.

♻️ This comment has been updated with latest results.

@couet
Copy link
Member Author

couet commented Jun 6, 2024

Therefore, histograms with a negative minimum will be rejected.

How it is done? Currently I see no protection for negative bins content.

In Add:

if (h1->GetMinimum() < 0.) {
       Error("Add","Histograms with a negative minimum cannot be part of a THStack");
       return;
}

@couet
Copy link
Member Author

couet commented Jun 6, 2024

Any way this PR make several test fail like "tutorial-legacy-mlp-mlpRegression" let see what @moneta think about it.

@couet
Copy link
Member Author

couet commented Jun 6, 2024

It seems OK now

Copy link
Member

@linev linev left a comment

Choose a reason for hiding this comment

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

One should change Warning method name.

@couet couet merged commit 78298a1 into root-project:master Jun 10, 2024
guitargeek pushed a commit to guitargeek/root that referenced this pull request Jun 13, 2024
* Update THStack documentation

* Only issue a warning in case of negative histogram in a stack

* Change the method name in the Warning call.
guitargeek pushed a commit that referenced this pull request Jun 13, 2024
* Update THStack documentation

* Only issue a warning in case of negative histogram in a stack

* Change the method name in the Warning call.
@couet couet deleted the neg-thstack branch August 15, 2024 06:44
silverweed pushed a commit to silverweed/root that referenced this pull request Aug 19, 2024
* Update THStack documentation

* Only issue a warning in case of negative histogram in a stack

* Change the method name in the Warning call.
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.

2 participants