Implement variable rebinning 2-D histogram classes (ROOT-5224) #5280
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.
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 17 files 17 suites 4d 7h 15m 24s ⏱️ 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
| @@ -107,7 +107,7 @@ class TH2 : public TH1 { | |||
| TH2 *RebinX(Int_t ngroup=2, const char *newname="") override; // *MENU* | |||
| 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.
this signature change could potentially break derived user-classes on preexisting code that use override keyword. Maybe a separate function with no default arguments is better?
Implement variable rebinning 2-D histogram classes (ROOT-5224)
Implemented for TProfile2D as well