Implement variable rebinning 2-D histogram classes (ROOT-5224)#5280
Implement variable rebinning 2-D histogram classes (ROOT-5224)#5280pamputt wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Hi, thanks for the work! I left some small formatting comments.
I also had a rough look at the code, and didn't see obvious mistakes, but I cannot test / am not an expert.
I would either
- Hand over to @couet or @lmoneta to check
- Or/and you could add a tiny test for rebinning. You can use hist/hist/test/THn.cxx as a template.
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
Thank you for adding this useful extension to the rebinning of the 2D histograms.
The important missing piece is a test. The best and simplest way is to add new functions in test/stressHistogram.cxx
There are two tests for 1d, testArrayRebin() and testArrayRebinProfile(), it would be needed to add similar functions for the 2D case.
See https://github.com/root-project/root/blob/master/test/stressHistogram.cxx#L8082
It would be also great if you have time to add this for the 3D case. But this could be eventually a different PR
Thank you
Lorenzo
Implemented for TProfile2D as well.
|
@phsft-bot build Let's see if we can revive this PR. |
|
Starting build on |
|
Build failed on ROOT-ubuntu2204/nortcxxmod. Failing tests: |
Test Results 4 files 4 suites 1d 2h 36m 26s ⏱️ For more details on these failures, see this check. Results for commit 42f2b3f. ♻️ This comment has been updated with latest results. |
Thank you for taking up this work again. I'd forgotten about it and I don't think I'll finish it myself. As far as I remember, I stopped when I was writing the test that was required. In the few cases that I tested by hand, it seemed to work but it's not at all impossible for the code to be buggy, hence the need to check it thoroughly with tests. |
|
No worries @pamputt, I didn't expect you to do any work here! I will take a look at this PR, and if it appears good to merge for me I will finish and merge this development myself. |
guitargeek
changed the title
| virtual TH2 *RebinY(Int_t ngroup=2, const char *newname=""); // *MENU* | ||
| TH2 *Rebin(Int_t ngroup=2, const char*newname="", const Double_t *xbins = nullptr) override; // re-implementation of the TH1 function using RebinX | ||
| virtual TH2 *Rebin2D(Int_t nxgroup=2, Int_t nygroup=2, const char *newname=""); // *MENU* | ||
| virtual TH2 *Rebin2D(Int_t nxgroup=2, Int_t nygroup=2, const char *newname="", const Double_t *xbins=nullptr, const Double_t *ybins=nullptr); // *MENU* |
There was a problem hiding this comment.
(There might be very rare cases where this signature change could potentially break derived user-classes on preexisting code that use override keyword, but doesn't seem to worrying.)
|
revived once again. |
|
@dpiparo unfortunately we can't proceed with this PR because the PR branch is called I have opened a follow up PR, which is for not an exact copy of this PR by @pamputt: Let's follow up with the review there and bring it to a merge-able state to finally close ROOT-5224. |
8 participants
Implement variable rebinning 2-D histogram classes (ROOT-5224)
Implemented for TProfile2D as well