Code review?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Code review?

Kenneth Sloan-2
Context: Java plugins
I want to take a completely arbitrary ImageProcessor and produce a ColorProcessor, with the twist that I want to enhance images for display.
For my images, most color images (including 8-bit images with color LUTs) are already sufficiently enhanced (or - the colors actually mean something and I don’t want to interfere with that).  But, the grayscale images include everything from 8-bit images with and without LUTs to 32-bit Float images with very strange ranges of values.

The intent is to produce an image to LOOK AT.  Measurements are done using parallel copies of the images, which are not modified (and not displayed).

I started with a very verbose set of cases, for testing and debugging.  I had some difficulty with the 32-bit images and found it necessary to convert (with scaling) to 16-bit before enhancing.  I don’t remember what the issue was.  Today, I decided to collapse the cases at the cost of possibly doing some extra work.  Speed and other efficiency is NOT an issue.

I would appreciate it if someone more expert than myself  would take a look at the following piece of code and  comment.  It appears to work properly, but I have really only tested it in 32-bit Float and 8-bit images with color LUTs.  I’m especially interested in comments on why the computation of min/max and the conversion to 16-bit is required for 32-bit images (and also whether it is “wrong” to do that for other image formats).  It appears to work, but this is the result of trial and error (lots of errors!) and I may have lost perspective.  Of course, this is a tiny part of (several) larger Java plugins.  I *think* that the use of IJ.run(…) is required for the “Enhance Contrast” step; some of the other steps might use more direct methods, but as long as I have the ImagePlus available, I thought I might as well use it.

I am grateful for any comments.

    // enhance an ImageProcessor for display purposes
    // Input is an arbitrary ImageProcessor
    // Output is a ColorProcessor, possibly enhanced for display
    private ColorProcessor ip2cp(ImageProcessor ipIn)
    {
        if(ipIn.isGrayscale())
            {
                ImageProcessor ip = ipIn.duplicate();
                ImagePlus ipl = new ImagePlus("temp", ip);
                ImageStatistics imStat = ip.getStatistics();
                double min = imStat.min;
                double max = imStat.max;
                ipl.setDisplayRange(min,max);
                IJ.run(ipl,"16-bit","true");
                IJ.run(ipl,"Enhance Contrast","saturated=0.35");
                IJ.run(ipl,"Apply LUT","");
                return ipl.getProcessor().convertToColorProcessor();
            }
        return ipIn.convertToColorProcessor();
    }


Kenneth Sloan
[hidden email]
Vision is the art of seeing what is invisible to others.

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Code review?

Fred Damen
Greetings Kenneth,

ImageJ has a option (Edit>Options>Conversions...:Scale when converting.)
that causes the FloatProcessor to be scaled to the display range when
converting to ShortProcessor.  By setting your FloatProcessor's
DisplayRange to max and min you are effectively disabling this feature.
When starting to process DICOM data this gave me a big headache.  I
grabbed the float[][] array and converted it to short[][] and created a
ShortProcessor using this short[][].

NB: ip.resetMinAndMax();

You can enhance your ImageProcessor using:
(new ContrastEnhancer).stretchHistogram(ip,0.35);

Although I do not know what the issues you were seeing during this
process, I assume that you would want to do the enhancement on the
FloatProcessor before the conversion to an integer format.  Also iff there
is a need to preconvert to an integer format before conversion to a color
format, you should probably convert to a ByteProcessor as that seems to be
the first step that is taken before converting to a color format.

As to why you seem to have issues with converting from FloatProcessor and
not others may derive from the use of an abstract method (createImage)
that creates a AWT Image (byte) that is passed to the ColorProcessor
constructor; So... each type of processor does it differently.  As you
were not specific as to the nature of your issue with the conversion it is
hard to comment further.

Enjoy,

Fred


On Sat, January 16, 2021 4:51 pm, Kenneth Sloan wrote:

> Context: Java plugins
> I want to take a completely arbitrary ImageProcessor and produce a
> ColorProcessor, with the twist that I want to enhance images for display.
> For my images, most color images (including 8-bit images with color LUTs)
> are already sufficiently enhanced (or - the colors actually mean something
> and I don’t want to interfere with that).  But, the grayscale images
> include everything from 8-bit images with and without LUTs to 32-bit Float
> images with very strange ranges of values.
>
> The intent is to produce an image to LOOK AT.  Measurements are done using
> parallel copies of the images, which are not modified (and not displayed).
>
> I started with a very verbose set of cases, for testing and debugging.  I
> had some difficulty with the 32-bit images and found it necessary to
> convert (with scaling) to 16-bit before enhancing.  I don’t remember
> what the issue was.  Today, I decided to collapse the cases at the cost of
> possibly doing some extra work.  Speed and other efficiency is NOT an
> issue.
>
> I would appreciate it if someone more expert than myself  would take a
> look at the following piece of code and  comment.  It appears to work
> properly, but I have really only tested it in 32-bit Float and 8-bit
> images with color LUTs.  I’m especially interested in comments on why
> the computation of min/max and the conversion to 16-bit is required for
> 32-bit images (and also whether it is “wrong” to do that for other
> image formats).  It appears to work, but this is the result of trial and
> error (lots of errors!) and I may have lost perspective.  Of course, this
> is a tiny part of (several) larger Java plugins.  I *think* that the use
> of IJ.run(…) is required for the “Enhance Contrast” step; some of
> the other steps might use more direct methods, but as long as I have the
> ImagePlus available, I thought I might as well use it.
>
> I am grateful for any comments.
>
>     // enhance an ImageProcessor for display purposes
>     // Input is an arbitrary ImageProcessor
>     // Output is a ColorProcessor, possibly enhanced for display
>     private ColorProcessor ip2cp(ImageProcessor ipIn)
>     {
> if(ipIn.isGrayscale())
>    {
> ImageProcessor ip = ipIn.duplicate();
> ImagePlus ipl = new ImagePlus("temp", ip);
> ImageStatistics imStat = ip.getStatistics();
> double min = imStat.min;
> double max = imStat.max;
> ipl.setDisplayRange(min,max);
> IJ.run(ipl,"16-bit","true");
> IJ.run(ipl,"Enhance Contrast","saturated=0.35");
> IJ.run(ipl,"Apply LUT","");
> return ipl.getProcessor().convertToColorProcessor();
>    }
> return ipIn.convertToColorProcessor();
>     }
>
> —
> Kenneth Sloan
> [hidden email]
> Vision is the art of seeing what is invisible to others.
>
> --
> ImageJ mailing list: http://imagej.nih.gov/ij/list.html
>

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Code review?

Kenneth Sloan-2
Fred-

Thank you for the reply.  My recollection is that simply  converting the Float processor to color produced black images for images which had an actual range of approximately [-0.5..+1.0].  I believe I first tried to convert to 16-bit without scaling (which I think failed) and then settled on the code you see in my last e-mail.  I more or less stumbled onto the code shown by tinkering until it produced the correct result.  I think I decided that the behavior suggested that  the conversion from Float to Color was using [0..255] as the display range.  I'm sure I tried setting min/max and converting from Float to Color - but that failed.  Only by converting first to 16-bit did it work.  Note that some of my Float images have a very small range when measured in integers, and half the range is negative.

In my most recent version  (before this one) I had a full if-then-else chain to select every possible image type.  Most of these eventually were pared down to a simple "Enhance" followed by "convertToColorProcessor".  So...I decided that the min/max computation and the conversion to 16-bit "couldn't hurt" and simplified the code.  I can even hallucinate reasons why this might be necessary for other image types - but I simply did not have images to test those conditions.  So...I made the conversion process uniform and (so far) it has worked as expected.

I may look into removing the necessity of creating an ImagePlus and using IJ.run(...) for some of the manipulations.  But, right now I'm primarily interested in opinions on whether the existing code will actually BREAK under some conditions that I haven't been able to test.  If someone has a theory that the code *might* fail on some particular image type, I can generate an appropriate test.  Until then, I'm limited to the images I have on hand (which cover all of my anticipated needs).  

Getting rid of the ImagePlus creation and the overhead of using IJ.run(...) to access functions that can be accessed directly using only an ImageProcessor would be an aesthetic nicety - but I don't really need to do it.  There are no space or time efficiencies that matter.

I'm also not sure why the "ApplyLUT" step is required (I have my theories, but only that).  Again - it works as is, so "in practice" I'm happy.  Alas, I feel the need to get the "in theory" part right, too!

Finally, given my background as a card-carrying bit-twiddler, I try to restrain myself from diving down into the "array of pixels" world.  I can do it - but I'm trying realHard to use the highest level of abstraction available.

Again - thank you for the reply.

--
Kenneth Sloan
[hidden email]
Vision is the art of seeing what is invisible to others.





> On Jan 16, 2021, at 22:04, Fred Damen <[hidden email]> wrote:
>
> Greetings Kenneth,
>
> ImageJ has a option (Edit>Options>Conversions...:Scale when converting.)
> that causes the FloatProcessor to be scaled to the display range when
> converting to ShortProcessor.  By setting your FloatProcessor's
> DisplayRange to max and min you are effectively disabling this feature.
> When starting to process DICOM data this gave me a big headache.  I
> grabbed the float[][] array and converted it to short[][] and created a
> ShortProcessor using this short[][].
>
> NB: ip.resetMinAndMax();
>
> You can enhance your ImageProcessor using:
> (new ContrastEnhancer).stretchHistogram(ip,0.35);
>
> Although I do not know what the issues you were seeing during this
> process, I assume that you would want to do the enhancement on the
> FloatProcessor before the conversion to an integer format.  Also iff there
> is a need to preconvert to an integer format before conversion to a color
> format, you should probably convert to a ByteProcessor as that seems to be
> the first step that is taken before converting to a color format.
>
> As to why you seem to have issues with converting from FloatProcessor and
> not others may derive from the use of an abstract method (createImage)
> that creates a AWT Image (byte) that is passed to the ColorProcessor
> constructor; So... each type of processor does it differently.  As you
> were not specific as to the nature of your issue with the conversion it is
> hard to comment further.
>
> Enjoy,
>
> Fred
>
>
> On Sat, January 16, 2021 4:51 pm, Kenneth Sloan wrote:
>> Context: Java plugins
>> I want to take a completely arbitrary ImageProcessor and produce a
>> ColorProcessor, with the twist that I want to enhance images for display.
>> For my images, most color images (including 8-bit images with color LUTs)
>> are already sufficiently enhanced (or - the colors actually mean something
>> and I don’t want to interfere with that).  But, the grayscale images
>> include everything from 8-bit images with and without LUTs to 32-bit Float
>> images with very strange ranges of values.
>>
>> The intent is to produce an image to LOOK AT.  Measurements are done using
>> parallel copies of the images, which are not modified (and not displayed).
>>
>> I started with a very verbose set of cases, for testing and debugging.  I
>> had some difficulty with the 32-bit images and found it necessary to
>> convert (with scaling) to 16-bit before enhancing.  I don’t remember
>> what the issue was.  Today, I decided to collapse the cases at the cost of
>> possibly doing some extra work.  Speed and other efficiency is NOT an
>> issue.
>>
>> I would appreciate it if someone more expert than myself  would take a
>> look at the following piece of code and  comment.  It appears to work
>> properly, but I have really only tested it in 32-bit Float and 8-bit
>> images with color LUTs.  I’m especially interested in comments on why
>> the computation of min/max and the conversion to 16-bit is required for
>> 32-bit images (and also whether it is “wrong” to do that for other
>> image formats).  It appears to work, but this is the result of trial and
>> error (lots of errors!) and I may have lost perspective.  Of course, this
>> is a tiny part of (several) larger Java plugins.  I *think* that the use
>> of IJ.run(…) is required for the “Enhance Contrast” step; some of
>> the other steps might use more direct methods, but as long as I have the
>> ImagePlus available, I thought I might as well use it.
>>
>> I am grateful for any comments.
>>
>>    // enhance an ImageProcessor for display purposes
>>    // Input is an arbitrary ImageProcessor
>>    // Output is a ColorProcessor, possibly enhanced for display
>>    private ColorProcessor ip2cp(ImageProcessor ipIn)
>>    {
>> if(ipIn.isGrayscale())
>>    {
>> ImageProcessor ip = ipIn.duplicate();
>> ImagePlus ipl = new ImagePlus("temp", ip);
>> ImageStatistics imStat = ip.getStatistics();
>> double min = imStat.min;
>> double max = imStat.max;
>> ipl.setDisplayRange(min,max);
>> IJ.run(ipl,"16-bit","true");
>> IJ.run(ipl,"Enhance Contrast","saturated=0.35");
>> IJ.run(ipl,"Apply LUT","");
>> return ipl.getProcessor().convertToColorProcessor();
>>    }
>> return ipIn.convertToColorProcessor();
>>    }
>>
>> —
>> Kenneth Sloan
>> [hidden email] <mailto:[hidden email]>
>> Vision is the art of seeing what is invisible to others.
>>
>> --
>> ImageJ mailing list: http://imagej.nih.gov/ij/list.html <http://imagej.nih.gov/ij/list.html>
>>
>
> --
> ImageJ mailing list: http://imagej.nih.gov/ij/list.html <http://imagej.nih.gov/ij/list.html>

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Code review?

Fred Damen
Greetings Kenneth,

Thanks for the thanks...

There are a number of ways to convert from FloatProcessor to others, and
in similar impetus to your initiating this thread I decided I could not
figure out the correct full proof way to accomplish this task using ImageJ
supplied code and rolling my own gave me the warm and fuzzies that it will
always work as I needed it to. I guess this is the difference between the
quick and dirty get answers ASAP VS hoping things still work after you
upgrade just before the your deadlines. YMMV...

Converting the float range [-0.5...+1.0) range to integer will results in
0 (zero) due to quantization, i.e., all back (or white depending on
another option).  I am guessing that the method you used when converting
to 16bit happened to scale, and the other way did not scale.  For your
purpose I would recommend using a method that explicitly scales OR set the
option to scale when converting, albeit make sure the method you choose
pays attention to this option.

When producing color images for display I have used without issues:
IJ.run(newimp,"RGB Color", "");

Fred

On Sat, January 16, 2021 10:53 pm, Kenneth Sloan wrote:

> Fred-
>
> Thank you for the reply.  My recollection is that simply  converting the
> Float processor to color produced black images for images which had an
> actual range of approximately [-0.5..+1.0].  I believe I first tried to
> convert to 16-bit without scaling (which I think failed) and then settled
> on the code you see in my last e-mail.  I more or less stumbled onto the
> code shown by tinkering until it produced the correct result.  I think I
> decided that the behavior suggested that  the conversion from Float to
> Color was using [0..255] as the display range.  I'm sure I tried setting
> min/max and converting from Float to Color - but that failed.  Only by
> converting first to 16-bit did it work.  Note that some of my Float images
> have a very small range when measured in integers, and half the range is
> negative.
>
> In my most recent version  (before this one) I had a full if-then-else
> chain to select every possible image type.  Most of these eventually were
> pared down to a simple "Enhance" followed by "convertToColorProcessor".
> So...I decided that the min/max computation and the conversion to 16-bit
> "couldn't hurt" and simplified the code.  I can even hallucinate reasons
> why this might be necessary for other image types - but I simply did not
> have images to test those conditions.  So...I made the conversion process
> uniform and (so far) it has worked as expected.
>
> I may look into removing the necessity of creating an ImagePlus and using
> IJ.run(...) for some of the manipulations.  But, right now I'm primarily
> interested in opinions on whether the existing code will actually BREAK
> under some conditions that I haven't been able to test.  If someone has a
> theory that the code *might* fail on some particular image type, I can
> generate an appropriate test.  Until then, I'm limited to the images I
> have on hand (which cover all of my anticipated needs).
>
> Getting rid of the ImagePlus creation and the overhead of using
> IJ.run(...) to access functions that can be accessed directly using only
> an ImageProcessor would be an aesthetic nicety - but I don't really need
> to do it.  There are no space or time efficiencies that matter.
>
> I'm also not sure why the "ApplyLUT" step is required (I have my theories,
> but only that).  Again - it works as is, so "in practice" I'm happy.
> Alas, I feel the need to get the "in theory" part right, too!
>
> Finally, given my background as a card-carrying bit-twiddler, I try to
> restrain myself from diving down into the "array of pixels" world.  I can
> do it - but I'm trying realHard to use the highest level of abstraction
> available.
>
> Again - thank you for the reply.
>
> --
> Kenneth Sloan
> [hidden email]
> Vision is the art of seeing what is invisible to others.
>
>
>
>
>
>> On Jan 16, 2021, at 22:04, Fred Damen <[hidden email]> wrote:
>>
>> Greetings Kenneth,
>>
>> ImageJ has a option (Edit>Options>Conversions...:Scale when converting.)
>> that causes the FloatProcessor to be scaled to the display range when
>> converting to ShortProcessor.  By setting your FloatProcessor's
>> DisplayRange to max and min you are effectively disabling this feature.
>> When starting to process DICOM data this gave me a big headache.  I
>> grabbed the float[][] array and converted it to short[][] and created a
>> ShortProcessor using this short[][].
>>
>> NB: ip.resetMinAndMax();
>>
>> You can enhance your ImageProcessor using:
>> (new ContrastEnhancer).stretchHistogram(ip,0.35);
>>
>> Although I do not know what the issues you were seeing during this
>> process, I assume that you would want to do the enhancement on the
>> FloatProcessor before the conversion to an integer format.  Also iff
>> there
>> is a need to preconvert to an integer format before conversion to a
>> color
>> format, you should probably convert to a ByteProcessor as that seems to
>> be
>> the first step that is taken before converting to a color format.
>>
>> As to why you seem to have issues with converting from FloatProcessor
>> and
>> not others may derive from the use of an abstract method (createImage)
>> that creates a AWT Image (byte) that is passed to the ColorProcessor
>> constructor; So... each type of processor does it differently.  As you
>> were not specific as to the nature of your issue with the conversion it
>> is
>> hard to comment further.
>>
>> Enjoy,
>>
>> Fred
>>
>>
>> On Sat, January 16, 2021 4:51 pm, Kenneth Sloan wrote:
>>> Context: Java plugins
>>> I want to take a completely arbitrary ImageProcessor and produce a
>>> ColorProcessor, with the twist that I want to enhance images for
>>> display.
>>> For my images, most color images (including 8-bit images with color
>>> LUTs)
>>> are already sufficiently enhanced (or - the colors actually mean
>>> something
>>> and I don’t want to interfere with that).  But, the grayscale
>>> images
>>> include everything from 8-bit images with and without LUTs to 32-bit
>>> Float
>>> images with very strange ranges of values.
>>>
>>> The intent is to produce an image to LOOK AT.  Measurements are done
>>> using
>>> parallel copies of the images, which are not modified (and not
>>> displayed).
>>>
>>> I started with a very verbose set of cases, for testing and debugging.
>>> I
>>> had some difficulty with the 32-bit images and found it necessary to
>>> convert (with scaling) to 16-bit before enhancing.  I don’t
>>> remember
>>> what the issue was.  Today, I decided to collapse the cases at the cost
>>> of
>>> possibly doing some extra work.  Speed and other efficiency is NOT an
>>> issue.
>>>
>>> I would appreciate it if someone more expert than myself  would take a
>>> look at the following piece of code and  comment.  It appears to work
>>> properly, but I have really only tested it in 32-bit Float and 8-bit
>>> images with color LUTs.  I’m especially interested in comments
>>> on why
>>> the computation of min/max and the conversion to 16-bit is required for
>>> 32-bit images (and also whether it is “wrong” to do that
>>> for other
>>> image formats).  It appears to work, but this is the result of trial
>>> and
>>> error (lots of errors!) and I may have lost perspective.  Of course,
>>> this
>>> is a tiny part of (several) larger Java plugins.  I *think* that the
>>> use
>>> of IJ.run(…) is required for the “Enhance Contrast”
>>> step; some of
>>> the other steps might use more direct methods, but as long as I have
>>> the
>>> ImagePlus available, I thought I might as well use it.
>>>
>>> I am grateful for any comments.
>>>
>>>    // enhance an ImageProcessor for display purposes
>>>    // Input is an arbitrary ImageProcessor
>>>    // Output is a ColorProcessor, possibly enhanced for display
>>>    private ColorProcessor ip2cp(ImageProcessor ipIn)
>>>    {
>>> if(ipIn.isGrayscale())
>>>    {
>>> ImageProcessor ip = ipIn.duplicate();
>>> ImagePlus ipl = new ImagePlus("temp", ip);
>>> ImageStatistics imStat = ip.getStatistics();
>>> double min = imStat.min;
>>> double max = imStat.max;
>>> ipl.setDisplayRange(min,max);
>>> IJ.run(ipl,"16-bit","true");
>>> IJ.run(ipl,"Enhance Contrast","saturated=0.35");
>>> IJ.run(ipl,"Apply LUT","");
>>> return ipl.getProcessor().convertToColorProcessor();
>>>    }
>>> return ipIn.convertToColorProcessor();
>>>    }
>>>
>>> —
>>> Kenneth Sloan
>>> [hidden email] <mailto:[hidden email]>
>>> Vision is the art of seeing what is invisible to others.
>>>
>>> --
>>> ImageJ mailing list: http://imagej.nih.gov/ij/list.html
>>> <http://imagej.nih.gov/ij/list.html>
>>>
>>
>> --
>> ImageJ mailing list: http://imagej.nih.gov/ij/list.html
>> <http://imagej.nih.gov/ij/list.html>
>
> --
> ImageJ mailing list: http://imagej.nih.gov/ij/list.html
>

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Code review?

Dimiter Prodanov (imec)
In reply to this post by Kenneth Sloan-2
Dear Ken,

Have you also tried first to convert to a ByteProcessor ?
When I resetMinAndMax before conversion I don't see issues with the range during conversion.

Best regards,

Dimiter

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Code review?

Kenneth Sloan-2
I have not tried that, yet.  Intuitively, it seems possible to lose
precision in cases where the “Enhance” step further narrows the range.
But, perhaps the conversion to Byte is itself more careful than the
conversion from 32-bit directly to Color.

I will try it. It looks like I should write a comprehensive set of test
cases to sort this out.

For that matter, I should probably try to replicate the failures I saw
earlier.  Perhaps the problem was with something unrelated to the
conversions.

I’m at the stage now where what I have works for the (moderately
challenging) set of test cases I have - but I don’t have a clear
understanding on WHY some steps are necessary.  Perhaps they are not?
All I know is that the 32-to-color case was broken and now works.  Now, I
should try to break it again.

This is a bit separate from my original question, which is: I know that
some(most) of my steps are NOT necessary for source images that are NOT
32-bit.  I was wondering if anyone had an opinion that these steps were
actually harmful (other than wasting time or space.

On the one hand, I come from a long tradition of counting every cycle and
every byte.  On the other hand I like straight line code that is easy to
read, without special cases.

Thank you for the suggestion.  I will experiment further.

On Mon, Jan 18, 2021 at 03:04 Dimiter Prodanov (imec) <
[hidden email]> wrote:

> Dear Ken,
>
> Have you also tried first to convert to a ByteProcessor ?
> When I resetMinAndMax before conversion I don't see issues with the range
> during conversion.
>
> Best regards,
>
> Dimiter
>
> --
> ImageJ mailing list: http://imagej.nih.gov/ij/list.html
>
--
-Kenneth Sloan

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html