Pixelmask

[mhantke]

Hi!
I united all masks to a static pixelmask (the same for all events) and a dynamic pixelmask (different for every event). They are 16 bit unsigned ints where every bit stands for a particular option defined in detectorObject.h. The set of options can be easily extended. Do you have any objections or remarks before merging?
I suggest the following convention:
/*

• Bits for pixel masks
• Along conventions defined for CXI file format ( https://github.com/FilipeMaia/CXI/raw/master/cxi_file_format.pdf )
• CONVENTIONS:
• - All options are dominantly inherited during assembly and pixel integration (see assembleImage.cpp)
• - The default value for all options is "false"
  */
  static const uint16_t PIXEL_IS_PERFECT = 0; // Remember to change this value if necessary when adding a new option
  static const uint16_t PIXEL_IS_INVALID = 1; // bit 0
  static const uint16_t PIXEL_IS_SATURATED = 2; // bit 1
  static const uint16_t PIXEL_IS_HOT = 4; // bit 2
  static const uint16_t PIXEL_IS_DEAD = 8; // bit 3
  static const uint16_t PIXEL_IS_SHADOWED = 16; // bit 4
  static const uint16_t PIXEL_IS_IN_PEAKMASK = 32; // bit 5
  static const uint16_t PIXEL_IS_TO_BE_IGNORED = 64; // bit 6
  static const uint16_t PIXEL_IS_BAD = 128; // bit 7
  static const uint16_t PIXEL_IS_OUT_OF_RESOLUTION_LIMITS = 256; // bit 8

Cheers,
max.

[rkirian]

Hi Max,

Sounds good to me. Did you also propagate the bit convention to saveFrame.cpp (see line 401)?

Best,
Rick

On Mar 28, 2013, at 8:21 PM, Max Hantke wrote:

Hi!
I united all masks to a static pixelmask (the same for all events) and a dynamic pixelmask (different for every event). They are 16 bit unsigned ints where every bit stands for a particular option defined in detectorObject.h. The set of options can be easily extended. Do you have any objections or remarks before merging?
I suggest the following convention:
/*

Bits for pixel masks
Along conventions defined for CXI file format ( https://github.com/FilipeMaia/CXI/raw/master/cxi_file_format.pdf )
CONVENTIONS:

• All options are dominantly inherited during assembly and pixel integration (see assembleImage.cpp)
• The default value for all options is "false" */ static const uint16_t PIXEL_IS_PERFECT = 0; // Remember to change this value if necessary when adding a new option static const uint16_t PIXEL_IS_INVALID = 1; // bit 0 static const uint16_t PIXEL_IS_SATURATED = 2; // bit 1 static const uint16_t PIXEL_IS_HOT = 4; // bit 2 static const uint16_t PIXEL_IS_DEAD = 8; // bit 3 static const uint16_t PIXEL_IS_SHADOWED = 16; // bit 4 static const uint16_t PIXEL_IS_IN_PEAKMASK = 32; // bit 5 static const uint16_t PIXEL_IS_TO_BE_IGNORED = 64; // bit 6 static const uint16_t PIXEL_IS_BAD = 128; // bit 7 static const uint16_t PIXEL_IS_OUT_OF_RESOLUTION_LIMITS = 256; // bit 8
  Cheers,
  max.

—
Reply to this email directly or view it on GitHub.

[mhantke]

Hi Rick!
Yes, I propagated the changes. The mask that is written to CXI/H5 is in accordance to the new conventions now and called "pixelmask" / "pixelmask_shared".
Cheers!

[antonbarty]

Hi Max

Masks are multiplied by the data to achieve masking effect. Is this compatible with a bit mask - where the integer value of the array is no longer 1 or 0? As I see it, when using a combined mask one would need additional logic operations to combine the different bits appropriately before applying the mask to the data. Is this really a good idea? Including the additional information is not a bad idea when saving the frames (this is done already) but it will necessitate a bunch of logical OR/AND/NOR operations whenever applying masks.

Comments?

A.

On Mar 28, 2013, at 8:21 PM, Max Hantke [email protected] wrote:

Hi!
I united all masks to a static pixelmask (the same for all events) and a dynamic pixelmask (different for every event). They are 16 bit unsigned ints where every bit stands for a particular option defined in detectorObject.h. The set of options can be easily extended. Do you have any objections or remarks before merging?
I suggest the following convention:
/*

Bits for pixel masks
Along conventions defined for CXI file format ( https://github.com/FilipeMaia/CXI/raw/master/cxi_file_format.pdf )
CONVENTIONS:

• All options are dominantly inherited during assembly and pixel integration (see assembleImage.cpp)
• The default value for all options is "false" */ static const uint16_t PIXEL_IS_PERFECT = 0; // Remember to change this value if necessary when adding a new option static const uint16_t PIXEL_IS_INVALID = 1; // bit 0 static const uint16_t PIXEL_IS_SATURATED = 2; // bit 1 static const uint16_t PIXEL_IS_HOT = 4; // bit 2 static const uint16_t PIXEL_IS_DEAD = 8; // bit 3 static const uint16_t PIXEL_IS_SHADOWED = 16; // bit 4 static const uint16_t PIXEL_IS_IN_PEAKMASK = 32; // bit 5 static const uint16_t PIXEL_IS_TO_BE_IGNORED = 64; // bit 6 static const uint16_t PIXEL_IS_BAD = 128; // bit 7 static const uint16_t PIXEL_IS_OUT_OF_RESOLUTION_LIMITS = 256; // bit 8
  Cheers,
  max.

—
Reply to this email directly or view it on GitHub.

[mhantke]

Hi Anton!
I believe we win as we reduce memory usage by not carrying 5 different integer masks or more through the process and the change makes the code more compact. If you want to apply a mask you just multiply by the bit/inverse you are interested in. I think this is not time critical but I should probably check the change in performance.
Best,
max.

[rkirian]

My feeling that it's bad practice to multiply the intensity data by the mask, though I often do that too. We should handle flagged pixels more explicitly - make the algorithms "aware" of the pixel-flagging convention. If my understanding of Max's convention is correct, then most algorithms would simply check if the corresponding pixel in the mask has a value of zero (all bits are zero), else the pixel is flagged for some reason and should be ignored.

Rick

On Mar 28, 2013, at 10:21 PM, Anton Barty wrote:

Hi Max

Masks are multiplied by the data to achieve masking effect. Is this compatible with a bit mask - where the integer value of the array is no longer 1 or 0? As I see it, when using a combined mask one would need additional logic operations to combine the different bits appropriately before applying the mask to the data. Is this really a good idea? Including the additional information is not a bad idea when saving the frames (this is done already) but it will necessitate a bunch of logical OR/AND/NOR operations whenever applying masks.

Comments?

A.

On Mar 28, 2013, at 8:21 PM, Max Hantke [email protected] wrote:

Hi!
I united all masks to a static pixelmask (the same for all events) and a dynamic pixelmask (different for every event). They are 16 bit unsigned ints where every bit stands for a particular option defined in detectorObject.h. The set of options can be easily extended. Do you have any objections or remarks before merging?
I suggest the following convention:
/*

Bits for pixel masks
Along conventions defined for CXI file format ( https://github.com/FilipeMaia/CXI/raw/master/cxi_file_format.pdf )
CONVENTIONS:

• All options are dominantly inherited during assembly and pixel integration (see assembleImage.cpp)
• The default value for all options is "false" */ static const uint16_t PIXEL_IS_PERFECT = 0; // Remember to change this value if necessary when adding a new option static const uint16_t PIXEL_IS_INVALID = 1; // bit 0 static const uint16_t PIXEL_IS_SATURATED = 2; // bit 1 static const uint16_t PIXEL_IS_HOT = 4; // bit 2 static const uint16_t PIXEL_IS_DEAD = 8; // bit 3 static const uint16_t PIXEL_IS_SHADOWED = 16; // bit 4 static const uint16_t PIXEL_IS_IN_PEAKMASK = 32; // bit 5 static const uint16_t PIXEL_IS_TO_BE_IGNORED = 64; // bit 6 static const uint16_t PIXEL_IS_BAD = 128; // bit 7 static const uint16_t PIXEL_IS_OUT_OF_RESOLUTION_LIMITS = 256; // bit 8
  Cheers,
  max.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[antonbarty]

On Mar 28, 2013, at 10:46 PM, rkirian [email protected] wrote:

My feeling that it's bad practice to multiply the intensity data by the mask, though I often do that too. We should handle flagged pixels more explicitly - make the algorithms "aware" of the pixel-flagging convention. If my understanding of Max's convention is correct, then most algorithms would simply check if the corresponding pixel in the mask has a value of zero (all bits are zero), else the pixel is flagged for some reason and should be ignored.

Ifs are generally bad practice in a loop - they cause problems with predictive branch execution, vectoring and pipelining - which slows down the code. It depends what one wants to do - the multiplication approach works where it works. Comparing not-zero-equals-bad is at least one operation, but can it be simplified to something more pipeline friendly.

[mhantke]

Yes, a pixel is "healthy" if the mask has a value of 0.
In most of the cases I don't see the reason why we "apply" masks during the execution of cheetah by setting the intensity value to 0. Personally I want to keep all information for the analysis later and gather information about the character of all pixels in a mask.
For hitfinding, radial averaging etc. we can look for a pipeline-friendly way of checking the bits in the loops.

[rkirian]

OK - I'll have to look up the meaning of "predictive branch execution" now :)

I'm just thinking of things like peakfinders, where there are loads of unavoidable if statements, and adding more seems negligible. But maybe this requires more thought than I realize.

Rick

On Mar 28, 2013, at 10:52 PM, Anton Barty wrote:

On Mar 28, 2013, at 10:46 PM, rkirian [email protected] wrote:

My feeling that it's bad practice to multiply the intensity data by the mask, though I often do that too. We should handle flagged pixels more explicitly - make the algorithms "aware" of the pixel-flagging convention. If my understanding of Max's convention is correct, then most algorithms would simply check if the corresponding pixel in the mask has a value of zero (all bits are zero), else the pixel is flagged for some reason and should be ignored.

Ifs are generally bad practice in a loop - they cause problems with predictive branch execution, vectoring and pipelining - which slows down the code. It depends what one wants to do - the multiplication approach works where it works. Comparing not-zero-equals-bad is at least one operation, but can it be simplified to something more pipeline friendly.
—
Reply to this email directly or view it on GitHub.

[FilipeMaia]

The way this is usually done is:

// zero hot pixels from image
for i in img_size:
img_zero_hot_pixels[i] = img[i] * ((mask[i] & PIXEL_IS_HOT) == 0)

which should be very CPU friendly as the bitwise and and comparison with zero can be done very efficiently in hardware.

[mhantke]

In my small example anton seems to be right:

// bitmask.cpp
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int BIT_SET = 16;
int L = 100000;

double diffclock(clock_t clock1,clock_t clock2)
{
double diffticks=clock1-clock2;
double diffms=(diffticks*10)/CLOCKS_PER_SEC;
return diffms;
}

int count_maskBit(int * maskBit)
{
int counter = 0;
for(int i=0;i<L;i++){
counter += ((maskBit[i] & BIT_SET)==0);
}
return counter;
}

int count_mask0(int * mask0)
{
int counter = 0;
for(int i=0;i<L;i++){
counter += mask0[i];
}
return counter;
}

int main()
{
clock_t begin;
clock_t end;
int * maskBit = (int_) calloc(L,sizeof(int));
int * mask0 = (int_) calloc(L,sizeof(int));
int * data = (int*) calloc(L,sizeof(int));
double dt;
int counter;

for(int i=0;i<L;i++){
mask0[i] = rand() % 2;
if (mask0[i] == 1) {
counter += 1;
}
else maskBit[i] = BIT_SET;
}

begin=clock();
for (int i=0;i<L;i++)
if (count_mask0(mask0)!=counter) printf("ERROR\n");
end=clock();
dt=diffclock(end,begin);
printf("Mask0 dt=%f\n",counter,dt);

begin=clock();
for (int i=0;i<L;i++)
if (count_maskBit(maskBit)!=counter) printf("ERROR\n");
end=clock();
dt=diffclock(end,begin);
printf("MaskBit dt=%f\n",counter,dt);

return 0;
}

hantke@gauguin:/temp/bitwise$ ./bitmask
Mask0 dt=316.200000
MaskBit dt=370.300000
hantke@gauguin:/temp/bitwise$ g++ -O2 bitmask.cpp -o bitmask
hantke@gauguin:/temp/bitwise$ ./bitmask
Mask0 dt=52.600000
MaskBit dt=107.500000
hantke@gauguin:/temp/bitwise$ g++ -O3 bitmask.cpp -o bitmask
hantke@gauguin:~/temp/bitwise$ ./bitmask
Mask0 dt=33.700000
MaskBit dt=107.500000

Pipelining seems to be difficult indeed for these bitwise operations.

[FilipeMaia]

That's comparing one mask. But if you have to check two masks or more (which is often the problem we have) then when using multiple masks the time goes up linearly while with the bitwise approach it almost doesn't increase (unless you go above 32 masks). In any case I think the time is irrelevant as we can mask 10 billion elements in a few seconds. IO will be a bottleneck way before this ever becomes an issue.

[mhantke]

I pushed my current version to a new branch. Feel free to play!

[mhantke]

Hi!
Now the branch 'pixekmask' is merged with Filipe's branch 'easybuild'. Both versions build with ccmake in Uppsala. I compared the performance and didn't notice a significant change in execution time.
Shall I merge with the master branch?
Cheers!

[antonbarty]

Hi Max

we should think a little about development and testing branches before merging everything into master.

When someone new clones the repository, they will start in the master branch.
This means Master should be the most stable branch available and not change too frequently (apart from the fixing of unintended bugs).

We should have a different branch for developers, which has the latest features for testing; this is where most of us will live most of the time.
When a set of new features is stable, debugged, and has been tested to work at both SLAC and CFEL (and Uppsala), it gets ported to the master branch. We do this not so frequently, and give Cheetah a new version number every time Master gets updated (minor bug fixes aside). Who knows who is downloading it in the meantime - Master should not result in any surprises or a deluge of 'this does not work' complaints. In other words, we need to test out a branch thoroughly ourselves before merging with master.

We could call this development branch 'developer' (for want of more creativity).

Comments welcome

Anton

On 02/04/2013, at 2:45 PM, Max Hantke [email protected] wrote:

Hi!
Now the branch 'pixekmask' is merged with Filipe's branch 'easybuild'. Both versions build with ccmake in Uppsala. I compared the performance and didn't notice a significant change in execution time.
Shall I merge with the master branch?
Cheers!

—
Reply to this email directly or view it on GitHub.

[mhantke]

Hi Anton!
That's a very reasonable suggestion! I created the branch 'developer'.
Cheers,
max.