[ROOT-5509] TH1::Merge() has problems when the histograms have no entries set. Created: 12/Sep/13  Updated: 15/May/19  Resolved: 07/Apr/14

Status: Closed
Project: ROOT
Component/s: Math Libraries
Affects Version/s: 5.34/00
Fix Version/s: None

Type: Bug Priority: High
Reporter: Andreas Bachlechner Assignee: Lorenzo Moneta
Resolution: Fixed Votes: 7
Labels: None
Environment:

All


Attachments: Text File 0001-Fixes-bugs-in-TH1-Merge-TH2-Merge-and-TH3-Merge.patch     File 0002-Fix-merging-of-histograms-with-non-equidistant-binni.patch     File th1MergeTest.cc     File th2MergeTest.cc     File th3MergeTest.cc    
Development:

 Description   

As TH1::Merge() skips histograms which do not have the entries set and by that has problems with merging these kind of histograms as it detects them at the moment as labeled histograms with non zero bin content (if only bin content is set but no entries).
In addition when the first histogram has no entries set and is skipped the merge function has problems calculating the correct axis ranges if both histograms do not have the exact same axis limits.
As a fix we could not skip the first histogram. We can look for the variable "initialLimitsFound" and check for this one before skipping this empty histogram.

So the old source

5211: do {
5212: // skip empty histograms
5213: if (h->fTsumw == 0 && h->GetEntries() == 0) continue;

would then become

5211: do {
5212: // skip empty histograms
5213: if (h->fTsumw == 0 && h->GetEntries() == 0 && initialLimitsFound) continue;

Anyhow there would be the question if skipping empty histograms is desired? What is the reason for skipping them? Especially if the histograms have different axis limits one might want to append also empty histograms. At the moment this is not possible so maybe it is worth a thought.



 Comments   
Comment by Andreas Bachlechner [ 16/Sep/13 ]

Short root script to test where TH1::Merge fails. The testA() and testB() functions demonstrate two different problems.

Comment by Andreas Bachlechner [ 17/Sep/13 ]

I would like to extend this bug to TH2 and TH3. They have the same problems of skipping empty histograms. In addition TH2 and TH3 Merge() fail if only one axis has different limits and the other has under or overflow entries. I added patches to solve this problems for TH1::Merge(), TH2::Merge() and TH3::Merge in the patchTHx.txt files.

Comment by Andreas Bachlechner [ 17/Sep/13 ]

Short root script to test where TH2::Merge fails. The testA() demonstrates to problem with one different axis and the other having overflow bins. testB() fails because the first histogram is empty.

Comment by Andreas Bachlechner [ 17/Sep/13 ]

Short root script to test where TH3::Merge fails. The testA() demonstrates to problem with one different axis and the other having overflow bins. testB() fails because the first histogram is empty.

Comment by Andreas Bachlechner [ 17/Sep/13 ]

Patch for fixing bug in TH1::Merge() (based on v5-34-09 git tag).

Comment by Andreas Bachlechner [ 17/Sep/13 ]

Patch for fixing bug in TH2::Merge() (based on v5-34-09 git tag).

Comment by Andreas Bachlechner [ 17/Sep/13 ]

Patch for fixing bug in TH3::Merge() (based on v5-34-09 git tag).

Comment by Andreas Bachlechner [ 18/Sep/13 ]

Patch for fixing bug in TH1::Merge() (based on v5-34-09 patches 3b5a128f146e14bf31a7f82a7b8de04ec4529be1).

Comment by Andreas Bachlechner [ 18/Sep/13 ]

Patch for fixing bug in TH2::Merge() (based on v5-34-09 patches 3b5a128f146e14bf31a7f82a7b8de04ec4529be1).

Comment by Andreas Bachlechner [ 18/Sep/13 ]

Patch for fixing bug in TH3::Merge() (based on v5-34-09 patches 3b5a128f146e14bf31a7f82a7b8de04ec4529be1).

Comment by Andreas Bachlechner [ 16/Oct/13 ]

[PATCH] Fixes bugs in TH1::Merge(), TH2::Merge() and TH3::Merge()

  • THx::Merge(): Do not skip empty histograms. The performance penalty is
    marginal and there are cases in which users are interested in such a
    merge. If they want to remove empty histograms, they should do so
    before entering the Merge function.
  • In TH2:: and TH3::Merge() the under and overflow bin entries create
    only problems if they are on an axis which does not have the same
    binning in all the merged histograms. If it has the same binning the
    entries in these bins can be merged normally. This is fixed by
    checking each axis individually for limit consistency.
  • TH1::RecomputeAxisLimits can now also merge an equidistant user
    binning.
Comment by Bastian Beischer [ 23/Oct/13 ]

[PATCH 2/2] Fix merging of histograms with non equidistant binning.

This is supposed to work if histograms can be merged in X, but have the
same binning in Y, which doesn't have to be equidistant!

Comment by Andreas Bachlechner [ 02/Dec/13 ]

Bump

Comment by Lorenzo Moneta [ 02/Dec/13 ]

Hi,

Sorry for not having replied to you earlier. Thank you for your patch. I can reproduce your problem, however, as you mentioned there is the problem with empty histograms.
My question is, do you really need this capability of merging histogram with different axis limits ? Do you think is really useful ? What is your use case for having this functionality ?
My opinion is by introducing something like this, one is going to have always a problem. Like what is going to happen if one of the merge histogram has underflow/overflows ?
I would be in favour of dropping this capability in new ROOT versions.

Cheers

Lorenzo

Comment by Bastian Beischer [ 02/Dec/13 ]

Hey Lorenzo,

First of all: Handling of histograms with under- and overflow bin contents is ok in ROOT as it is and we didn't touch it with the patch. The ROOT documentation clearly states what is done in such cases and we agree with the rationale described there. In fact ROOT already handles merging of histograms with different axis limits relatively well and it's all described in the ROOT docu. I don't see the reason to remove that functionality.

The reason we opened these bug reports is that there a few bugs in the current merge implementation which affect some corner cases.

Now let me give one example in which we would like to merge histograms with different axis limits.

We are analyzing large sets of data and want to keep the per process memory consumption low. For example, many of our resulting histograms are "time histograms" with 20 minute bins on the x-axis. The content of the y and z-axis can be used defined. Imagine you are using a TH3F with 20 minute bins on the x-axis which spans over a range of 2 years and has 100x100 bins on the y and z-axis. A TH3F would then take about 2GB of memory.

Now, on large computing cluster batch jobs will only process a given range of runs of scientific data. Then, for that particular process, there is no reason to create histograms which span over the whole time range used for analysis. Instead, if you divide the data in say 1000 batch jobs, the memory required for each individual TH3F goes down to 2MB which is fine for an individual batch job running on a worker node.

After everything finished, hadd can be used to merge the results. Since the binning is "consistent" in the sense that only 20 minute bins are used and they are created such that batch job j just fills the bins which directly follow the ones on batch job (j-1).

Now here's the important part: ROOT is in principle already able to merge these histograms in the way we would like it, but it has the following problems:

a) if the first histograms are empty there is a bug
b) if the y- or z-axis have a userdefined binning (say logarithmic binning) the merge does not go well, even though that axis is the same across all histograms
c) if the x-axis is "user defined" (using int nBins, double* bins) but still "compatible" for an axis merge, ROOT refuses to merge them and that makes no sense.

All of these issues were addressed with our patch and we didn't observe any slowdown, crashes or increase of memory consumption so I don't see any problem with our fixes.

There are other examples in which merging of histograms with different axis limits can be useful. We'll provide more if you're interested.

Let us know what you think.

Comment by Lorenzo Moneta [ 02/Dec/13 ]

Hi Bastian,

I agree with you in case of time histograms, where the axis is automatically extended in case of overflows, so by definition you never have underflow/overflow. In this case one should support the merging of different axis.
But what in case of normal histograms ? I have doubts in such cases, if you have an example there, I would be certainly interested.
Do you have also a test program showing the problems in the case b) and c) you described above ?

Cheers

Lorenzo

Comment by Lorenzo Moneta [ 02/Dec/13 ]

Hi,
Looking more at the test you have, the problem is caused by merging histogram with different bin width, and not by the fact that the first histogram is empty. If you change the order of merging the test will fail, even with your patch. So, merging histogram with different bin width is really useful ?
We can of course support everything, but it requires time and complex code and then effort to maintain it.
I agree, however, for user defined binning, if we support the merging for different histogram axes, we should do also for user defined bins, when they are the same. Your test will be however time consuming, so again, it should be done only for time histograms, so when the histograms have the option that their axis can be extended (as in case of label histograms)

Cheers

Lorenzo

Comment by Andreas Bachlechner [ 13/Dec/13 ]

Hi,

I am sorry for being late to answer. So now to your questions.

The time histograms where just one example for merging different axis. In principle the THx::Merge() function works properly for a wide range of bindings and histograms with different bin widths, bin ranges and handles also the under and overflow correctly but some problems with special cases (empty histograms and user defined bindings as they where simply excluded in the loop before).

Do you have also a test program showing the problems in the case b) and c) you described above ?

This is demonstrated in the userDefindedBinningMerge.cc script. Where we try to merge different histograms with normal and user defined binnings. These all work after the patch (both patches have to be applied).

Looking more at the test you have, the problem is caused by merging histogram with different bin width, and not by the fact that the first histogram is empty. If you change the order of merging the test will fail, even with your patch. So, merging histogram with different bin width is really useful ?

I tried the testA() in th1MergeTest.cc and changed the order of merging a few times and it is working after the patch(both patches have to be applied). In principle here again root works as expected and handles even different bin widths right but the empty histogram is a problem as they are skipped before.

I agree, however, for user defined binning, if we support the merging for different histogram axes, we should do also for user defined bins, when they are the same. Your test will be however time consuming, so again, it should be done only for time histograms, so when the histograms have the option that their axis can be extended (as in case of label histograms)

Well the check only applies when merging a user defined axis and before they just did not work so it does not slow down anything existing. It should also work for non time axis if one has a user defined equidistant binning for example and wants to merge these.

Cheers,
Andi

PS I had problems uploading the script so i paste the source for userDefindedBinningMerge.cc here:

#include <TH1.h>
#include <TList.h>
 
void testA() {
 
  TH1D* h1 = new TH1D("h1A", "h1A", 5, 2, 4.5);
  h1->Fill(3);
  TH1D* h2 = new TH1D("h2A", "h2A", 3, 4.5, 6);
  h2->Fill(5);
  TList list;
  list.Add(h1);
  // This works as both axis have a not user definded binning.
  h2->Merge(&list);
}
 
void testB() {
 
  double binning1[6] = {2, 2.5, 3, 3.5, 4, 4.5};
  TH1D* h1 = new TH1D("h1B", "h1B", 5, binning1);
  h1->Fill(3);
  TH1D* h2 = new TH1D("h2B", "h2B", 3, 4.5, 6);
  h2->Fill(5);
  TList list;
  list.Add(h1);
  // This fails as h1 has a user defined binning.
  // But as it is still the same equidistant binning it should work for this.
  // So TH1::RecomputeAxisLimits in TH1::Merge() has not only to check if the
  // binning is user defined but in addition allow an equidistant user binning.
  h2->Merge(&list);
}
 
void testC() {
 
  double binning1[6] = {2, 2.5, 3, 3.5, 4, 4.5};
  double binning2[4] = {4, 4.5, 5, 5.5};
  TH1D* h1 = new TH1D("h1C", "h1C", 5, binning1);
  h1->Fill(3);
  TH1D* h2 = new TH1D("h2C", "h2C", 3, binning2);
  h2->Fill(5);
  TList list;
  list.Add(h1);
  // This fails as again h1 and h2 have user defined binning.
  h2->Merge(&list);
}

Comment by Andreas Bachlechner [ 13/Dec/13 ]

Hi Lorenzo,

I am currently at CERN so if you have time I could visit you in your office to discuss if you have more questions regarding this patch.

Cheers,
Andi

Comment by Andreas Bachlechner [ 19/Jan/14 ]

Bump Do you need more information?

Comment by Andreas Bachlechner [ 23/Feb/14 ]

We would really like to help solving this bug in ROOT.

Comment by Lorenzo Moneta [ 04/Apr/14 ]

Your patch is now committed in the HEAD of 5.34 patches.
I have only added the case of skipping empty histograms during the merge operation. This should be faster when merging list with several empty histograms.

Please double-check if the current committed version works in your case

Sorry for having missed your previous JIRA messages

Thanks for the contribution

Lorenzo

Comment by Bastian Beischer [ 05/Apr/14 ]

Thank you Lorenzo!

It's very nice that our patches made it into v5-34-00-patches.

I'm not sure if your last patch was good, though. Andreas reported that your last patch broke merging for him. But looking at your change I can't see anything wrong with it. We'll have to debug a bit tomorrow and will provide you with the case that fails to merge correctly after your patch.

(Specifically we are filling histograms for passed / failed events vs time for cuts and then later on construct TEfficiencies out of these histograms. Andreas told me that with the tip of v5-34-00-patches the passed and failed histograms are inconsistent, such that we can't create the TEfficiency object. We didn't encounter this problem with the two patches we provided, so it must be the change you commited on top. As I said, we'll provide details tomorrow.)

Comment by Bastian Beischer [ 05/Apr/14 ]

OK I found the issue and fixed it.

Problem was that you introduced a new local variable called "histEntries" of type Int_t, but our histograms have large number of entries > INT_MAX and actually hist->GetEntries() returns Double_t, so histEntries should have been of type Double_t.

After changing Int_t histEntries = hist->GetEntries() to Double_t histEntries = hist->GetEntries() I think everything works. This needs to be changed in TH1::Merge(), TH2::Merge() and TH3::Merge().

Cheers
Bastian

Comment by Lorenzo Moneta [ 07/Apr/14 ]

Thanks for testing and to find and fix this last problem.
I close then now this bug report. In case you have still a problem, please let me know

Cheers

Lorenzo

Generated at Wed Sep 18 16:06:57 CEST 2019 using Jira 7.13.1#713001-sha1:5e06076c2d215a6f699b7e5c90ab2fae7ba5a1ce.