[hist] Enable chi2 tests for TProfile #19930
Conversation
Test Results 21 files 21 suites 3d 20h 44m 56s ⏱️ For more details on these failures, see this check. Results for commit e1a1548. ♻️ This comment has been updated with latest results. |
hist/hist/src/TProfile.cxx
Outdated
| { | ||
| TString opt = option; | ||
| opt.ToUpper(); | ||
| opt += "WW"; |
There was a problem hiding this comment.
should this happen only if h2 is a TProfile?
Actually, does it make sense to compute a chi2 between a TProfile and a non-TProfile?
There was a problem hiding this comment.
Generally, you could compare something that's not a TProfile with a TProfile, but I don't see how this makes sense. I was thinking that users either compare "normal" histograms with normal histograms, or profiles with profiles. For mixed cases, it probably depends on the case whether this makes sense.
On second thought, I agree that this modification of the options is safe only when h2 is also a profile. I'll change the code accordingly, and leave the options for the mixed cases as the user inputs them.
There was a problem hiding this comment.
ok, maybe you can consider overloading only the case where the other histogram is actually a TProfile and not a generic TH1.
There was a problem hiding this comment.
I considered this, but an overload TProfile::Chi2Test(TProfile*, ...) could not be called via a TH1 pointer. So this one
TH1* profile = new TProfile(...);
profile->Chi2Test( new TProfile(...) );would call TH1::Chi2Test(TH1*). This might be OK if users use their TProfiles always as TProfiles, but maybe polymorphism isn't such a bad thing here, when you e.g. want to collect histograms and profiles in the same container.
For this reason, I tend towards overriding instead of overloading. That would get us back to casting h2, and if the cast succeeds, the options get set to WW. If it fails, the users can choose any option they desire, like you did with the original version of the function.
The test can only work if the uncertainties are taken into account correctly, so the Chi2Test function was overridden for all TProfile classes. Furthermore, the function will check if the profiles have the correct error option set. See also the discussion in: https://root-forum.cern.ch/t/chi2test-using-tprofile/64156/
hageboeck
force-pushed
the
TProfileChi2Test
branch
from
987e2dc to
e1a1548
Compare
In reaction to this question
https://root-forum.cern.ch/t/chi2test-using-tprofile/64156
Note: I decided to ignore clang-format for the headers, because the indentation would look out of place.