Bug or feature in TColor?

ROOT
1

Hi,

I believe I have discovered an unusual and possibly undesirable behavior in TColor::SetPalette() in a recent checkout of the ROOT git head (5.99/03). This issue arises when SetPalette is called repeatedly using 0 (NULL) for the colors argument, in order to select one of the predefined colour palettes. For example assume that the ncolors argument is set to 51 to select the deep sea palette, (with reference to the source file accessible from the online documentation root.cern.ch/root/html/src/TColor.cxx.html#1660), the if block beginning on line 1660 contains an if statement on line 1662, which checks whether [i]ncolors/i equals fgPalette.fN (and also checks that the static member paletteType equals 3). This if statement appears to be intended to prevent unnecessary reinitialization of a color palette that is already defined. Indeed, if both criteria are fulfilled then TColor::SetPalette() returns immediately. Otherwise, execution proceeds and ultimately results in a call to TColor::CreateGradientColorTable().

As part of its operation, the static method TColor::CreateGradientColorTable() searches the global list of colours for the highest color index that has been allocated so far (potentially a very expensive operation if many, possibly duplicated, colours have already been defined) before defining a new palette of (in this case 255) colours and appending their indices to the global list as a a side-effect of the TColor constructor. Finally, TColor::CreateGradientColorTable() calls TColor::SetPalette(), this time with ncolors == 255 and colors != 0. Unfortunately, this results in fgPalette.fN being set to 255. Accordingly, subsequent calls to TColor::SetPalette() with ncolors == 51 will not pass the criteria required by the if statement on line 1662, ultimately resulting in 255 additional color indices being defined in the global list of colours and the search for the highest colour index becoming progressively more expensive on each repeated call. Indeed, loops in which standard color palettes are repeatedly set exhibit significant slowdown.

For now, I have been able to work around this issue/feature by storing the zeroth index of the generated color palette after the first call to TColor::SetPalette and manually checking whether this equals the zeroth index of the currently defined palette i.e:

if(gStyle->GetColorPalette(0) != oldZerothIndex){ gStyle->SetPalette(51, 0); oldZerothIndex = gStyle->GetColorPalette(0); }
where oldZerothIndex is initialised to -1 before the first call to SetPalette(). Note that gStyle->SetPalette() simply forwards its arguments to TColor::SetPalette().

I have a feeling that this is a bug in TColor, but perhaps there is some reason why this behaviour is desirable under certain circumstances? It seems like the static member paletteType should be sufficient to catch attempts at reinitialization of a predefined colour palette (except that both the user defined palette and the Grey Scale colour palette share paletteType==4.)

Perhaps I’m missing something? Is there a better way to avoid the slowdowns that I’m seeing?

Cheers,

Hugh

2

Thanks for your details analysis.
Do you have a example reproducing this slowdown ?

3

Although it is admittedly contrived, the most minimal example that would demonstrate the behaviour (at least on my system) would be:

[code]#include
#include <TStyle.h>
#include <TApplication.h>

int main(int argc, char * argv[]){
TApplication app(“app”, &argc, argv);
for(int i = 0; i < 50; i++){
gStyle->SetPalette(51, 0);
std::cout << i << std::endl;
}
app.Run(true);
return 0;
}
[/code]
Where the time between each integer being printed increases noticeably.
Cheers,

Hugh

4

Thanks. It is very clear with the following small macro run interactivelly
in ROOT. I will check:

{
   for(int i = 0; i < 100; i++){
      gStyle->SetPalette(51, 0);
      std::cout << i << std::endl;
   }
}
5

It looks like the “return tests” for predefined palettes should be on the palette type only.
I will check …

6

This is now fixed in 5.34-patches and in the trunk.
Thanks for reporting.