search

hadim / read-roi

Read ROI files .zip or .roi generated with ImageJ.
BSD 3-Clause "New" or "Revised" License
51 stars 22 forks source link

BUG: int16 for top/left/bottom/right #10

Closed scottclowe closed 5 years ago

scottclowe commented 5 years ago

Previously, you were reading in signed int16's for top and left by reading them in as uint16 and then moving them into the negative regime if the value exceeded 6000 (exclusive). However, that threshold should be at 2^16 = 65536 (inclusive).

Also, the bottom and right values are in uint16 encoding, but were being read in as int16.

hadim commented 5 years ago

Thanks for this PR!

How did you spot the bug?

I wonder if you could add a test that reproduces the bug before this PR.

hadim commented 5 years ago

If you have a small ROI file you can add it to read_roi/test/data/

scottclowe commented 5 years ago

I don't have a test file, sorry.

I am a maintainer for fissa, which has code reading in ImageJ rois at https://github.com/rochefort-lab/fissa/blob/master/fissa/readimagejrois.py

We based our code on https://gist.github.com/luispedro/3437255 and https://github.com/losonczylab/sima/blob/master/sima/misc/imagej.py. This code didn't discern between uint16 and int16 in the encoding. Our experimentalist college had problems analysing some of the ROIs; on investigation some polygon box edges were coming out with enormous values, greater than 32768. It transpired that whenever a ROI was on the edge of the field of view, they selected it with a box whose edge outside the boundary of the GUI display (escaping it to the left-hand side, in this case). Instead of truncating at 0, ImageJ encoded the location as a negative number. The ImageJ reading code was expecting a uint16, and after digging around I changed it to read in signed integers for the co-ordinates instead.

Recently, I managed to get unit-testing working on Windows with AppVeyor. That revealed there were some problems with the way we were reading in the values where it was somehow making an overflow as we moved the int32 to float64. I'm not quite sure what the cause of the problem is, but I looked around for newer implementations and found your package. Knowing you will probably have based your code on the same implementation as the one we did, I checked to see if yours had the bug I fixed a long time ago.

As I was writing this explanation, I realised my PR was wrong as I had entered 2^16 as the threshold instead of 2^15, which is what it should be. I've fixed that and forced-pushed now.

scottclowe commented 5 years ago

Unfortunately, I don't have a means of creating a ROI file to serve as test data.

hadim commented 5 years ago

It's fine. Thanks for the fix again!

scottclowe commented 5 years ago

No problem. Thanks for the speedy response!