patchGNU Octave - Patches: patch #9281, Image package: imsharpen

 
 

patch #9281: Image package: imsharpen

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Mon 06 Mar 2017 06:34:29 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 11 Apr 2017 10:21:26 PM UTC, comment #9: 

Thanks guys.  I pushed

http://hg.code.sf.net/p/octave/image/rev/f3f77f701848

http://hg.code.sf.net/p/octave/image/rev/b7fdda6a49a1

http://hg.code.sf.net/p/octave/image/rev/03cf78c6c0d6

Support for ND images is kind of there.  If imfilter ever gets around to support it, imsharpen will automatically support it too.

Carnë Draug <carandraug>
Group Member
Wed 05 Apr 2017 07:52:28 PM UTC, comment #8: 

Running our imsharpen code in Matlab (after making all the
changes to make it run in Matlab) gave the same results,
using Matlab rgb2lab/lab2rgb so, it is not a rgb2lab/lab2rgb problem.
Probably Matlab is doing something else, which is not documented.

I think that we should keep it like this, with xtest tag.

P.S.

Hartmut: Thanks for the detailed explanation

Carnë: please add your name to the copyright notice.

Avinoam Kalma <avinoam>
Group Member
Tue 04 Apr 2017 08:19:58 PM UTC, comment #7: 

When I test the latest version of imsharpen (see comment #5) and add the "double" command to the call to rgb2lab (as proposed in comment #6), then I still see the very same tests failing. But many more tests also fail, and the failing test fail by a much bigger amount (bigger than just a value of 1).

The reason for this is: Using the "double" command on an uint8 image gives you floating point numbers, but still in the range of 0...255. This is not the kind of input values that rgb2lab wants to have. So I think it will not treat the values >1 properly and just use a very, very dark input image. I think this is no bug of rgb2lab (I expect Matlab does the very same), it's just that you should better use the "im2double" command for this type conversion. (But even this is unnecessary in this case, since rgb2lab does this conversion itself if necessary.)

I would assume that the remaing failing tests (failing by a difference of 1) are due to some slight discrepancy between the calculation methond in Avinoam's imsharpen.m and the Matlab version. I would guess that Matlab adds the number 1 somewhere explicitly in the code. (But I would love to be proven wrong, Avinoam. So go ahead and double check this.) If this turns out to be the case, then I am not sure if I really want to copy the Matlab behavior, or instead better stick to the "text book" version, that Avinoam's code properly mimics. We could then make a small note of this in the help string.

Hartmut <hardy>
Tue 04 Apr 2017 07:52:35 PM UTC, comment #6: 


Carnë: Thanks for fixing the code.
Can you explain why the change


Lab = rgb2lab (double(I));


makes some failing tests to pass, but makes other passing tests to fail?

I will check if the failing test are due to lab2rgb/rgb2lab problem.

Avinoam Kalma <avinoam>
Group Member
Tue 04 Apr 2017 07:31:20 PM UTC, comment #5: 

I first made a bunch of style changes (see inlined comments):

https://bitbucket.org/carandraug/octave-image/commits/da6638b3ce30000abbd148d5f64b82ff7f29cb17

And then this for reduced memory usage and performance

https://bitbucket.org/carandraug/octave-image/commits/447fa23083a2187e88aad4672b6d87504319e339

Reuse existing variables with inplace operators (less memory and faster operations). Also, instead of multiplying by a logical matrix to set elements to zero, it's better to just use the logical matrix to index and assign values to zero.

The failing tests are all for rgb images. Does the Matlab incompatibilities come from rgb2lab or maybe lab2rgb?  Sounded like it from comment #3.  Could we make those failing tests there too?


Carnë Draug <carandraug>
Group Member
Sat 11 Mar 2017 09:38:14 PM UTC, comment #4: 

The new version of the m-file seems to have adressed all issues that popped to my eye, originally (see comment #1). The file looks alright to me know (but I haven't done any systematic comparison with Matlab results.) Let's wait for Carne's feedback now.

Regarding the "new problem" (at the end of comment #3): I think you should use im2double(I) instead of double(I) here. But when I do this, it doesn't change any test results at all. I think that rgb2lab does this type conversion internally already, so there is no need to do it once again in imsharpen.m

Are the remaining differences to Matlab results always of magnitude +1? Maybe Matlab does some special treatment here, and adds a (non-standard) +1 values somewhere? If this is really the case, would we like to do this as well, or better stick to the text book version (without any +1 deal)? Or do you know any reliable source to justify a +1 special treatment during an "unsharp masking" algorithm?

Hartmut <hardy>
Sat 11 Mar 2017 08:57:32 PM UTC, comment #3: 

Attached a better version, fixed according to comment #1 (Thanks Hartmut).

  • I changed the tolerance of the failing test to 1, so it will not fail, but still, there is 1 gray level difference between my results and Matlab results.


  • texinfo was fixed.

 

  • Line 54: copy/paste error was fixed.


  • I removed the clipping. I did not find differences after the removal.


  • More example were added for uint8 input.


  • Wrong indentation was fixed.


  • I added more tests for few values of "Amount". I hope it checks the Gaussian size.



And more more problem: I have tried to change


 Lab = rgb2lab (I);


to


 Lab = rgb2lab (double(I));

 
It fixed one test, but in made other test to fail. Any idea about this?

Please review the attached new version. Thanks.



(file #39973)

Avinoam Kalma <avinoam>
Group Member
Tue 07 Mar 2017 12:18:03 PM UTC, comment #2: 


Thanks for your comments.
I will prepare a new version.

Avinoam Kalma <avinoam>
Group Member
Mon 06 Mar 2017 08:01:29 PM UTC, comment #1: 

Thanks for this new function.

Some comments from my side:

  • One test is (slightly) failing. Should we make it an xtest? (This depends on Carne's preference how to deal with those cases.)


  • I get some texinfo errors on "help imsharpen": The "item" bullet points seem to be used outside of their intended context. And there might also be some missing curly brackets in the help text.


  • line 54: There seems to be a copy and paste error, this error messages claims to be throughn by "impyramid".


  • line 106,107: Is this clipping of L to the interval [0,100] Matlab compatible? (My experience from implementing the color conversion functions is, that Matlab normally does not clip, but just returns useless results. But I haven't checked the Matlab behavior in this very case.)


  • some (one?) additional test with useful input images of class uint8 might be good. For example: A=zeros(7,7,'unit8'); A(4,4)=30; imsharpen(A);


  • I am no expert on coding guidelines in the image package. But the missing identation of function blocks (2 space signes per line) popped to my eye. There might be more code style issues.


  • line 62-65: The calculation of imsharpen_size looks funny. Is this Matlab compatible? Could you add a test for this (somehow)?
Hartmut <hardy>
Mon 06 Mar 2017 06:34:29 PM UTC, original submission:  


Attached imsharpen.m for Image Package.
Please review

Avinoam Kalma <avinoam>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #39973:  imsharpen.m added by avinoam (9KiB - application/octet-stream)
file #39905:  imsharpen.m added by avinoam (7KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by avinoam (Submitted the item)
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by avinoam
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-11 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2017-03-11 avinoam Attached File- Added imsharpen.m, #39973
    2017-03-06 avinoam Attached File- Added imsharpen.m, #39905
        Carbon-Copy- Added -email is unavailable-
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code