bugGNU Octave - Bugs: bug #50153, image package: bwperim result on...

 
 

bug #50153: image package: bwperim result on 1x1 input incorrect

Submitted by:  Hartmut <hardy>
Submitted on:  Thu 26 Jan 2017 12:50:57 PM UTC  
 
Category:  Octave Forge Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  4.2.0 Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Mon 03 Apr 2017 12:50:20 PM UTC, comment #10: 
Carnë Draug <carandraug>
Project Member
Sun 02 Apr 2017 05:53:53 AM UTC, comment #9: 

Checking also Carnë's patch from comment #8, I got the
same results as Hartmut (comment #8), and the same conclusion.

Avinoam Kalma <avinoam>
Project Member
Sat 01 Apr 2017 09:37:49 PM UTC, comment #8: 

I have read through Carne's proposed patch in comment #7. To be honest, I didn't quite understand all in the convoluted section (as the code itself names it) where one check was dropped. But

  • the patch fixes the original issue in comment #0
  • all tests still pass
  • it adds some (passing) tests, including a new test for the issue in comment #0
  • the approach itself sound reasonable to me

So I would be happy to see this patch applied to the image repo. Thanks for working this out, Carne!

Hartmut <hardy>
Mon 27 Mar 2017 05:06:14 PM UTC, comment #7: 

The proposed patch would error on the cases where connectivity is just [0 0 0; 0 1 0; 0 0 0]. A very corner case but it hints that the issue is not 1x1 image. I think the issue is more generalised about not considering border elements in dimensions of length 1. This alternative patch

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

seems to fix the original issue plus several new ones (such as image of `true(1, 1, 6)`) This just removes a check that was just wrong.  But maybe the check was there for some reason that we have no test for.  I think the test was just wrong (mea culpa) so if no one can see anything wrong with it, I will push that cset.

Carnë Draug <carandraug>
Project Member
Mon 30 Jan 2017 08:45:06 PM UTC, comment #6: 

I have tested the (second) patch from comment #5 (file #39615):

  • The new test for the use case in comment #0 passes. So it fixes the original issue.
  • All other tests still pass.

I would support to commit this patch to the image repository. Thanks, Avinoam!

Hartmut <hardy>
Mon 30 Jan 2017 08:26:26 PM UTC, comment #5: 

Indeed there was a mistake in my patch, sorry.
Here is the fixed patch with a test.

(file #39615)

Avinoam Kalma <avinoam>
Project Member
Mon 30 Jan 2017 07:54:16 PM UTC, comment #4: 

I have tested Avinoam's patch from comment #1:

  • There is a typo in the line with the special case. The code there should probably read "perim = bw;", currently this is swapped in the patch.
  • Could you add a small test for the use case in comment #0?
Hartmut <hardy>
Mon 30 Jan 2017 07:11:31 PM UTC, comment #3: 

The fix from comment #1 attached as a patch.

(file #39614)

Avinoam Kalma <avinoam>
Project Member
Mon 30 Jan 2017 06:13:38 PM UTC, comment #2: 

I don't have any objections against a "quick fix" solution, that simply introduces a special case for single pixel inputs.

(@Avinoam: But I don't quite recognize the current code of bwperim.m in your proposed "patch" in comment #1.)

Hartmut <hardy>
Mon 30 Jan 2017 05:51:19 PM UTC, comment #1: 

I have check all 2x2 logical matrices, and some 2x3 and 3x3,
and it seems that bwperim is correct in all of them.
The only exception is 1x1, so what about a trivial check:

  if (numel(bw) == 1)
    perim = bw;
  else
    perim = (! imerode (bw, conn)) & bw;
  end

Avinoam Kalma <avinoam>
Project Member
Thu 26 Jan 2017 12:50:57 PM UTC, original submission:  

This happens with Octave 4.2.0 and the current image-2.6.1.

The result of this little script

BW = true;
P = bwperim(BW)
Q = bwperim(BW, 8)

returns twice a single 0 in Octave, but it returns twice a 1 in Matlab. The Matlab result makes more sense to me, since even a 1x1 region DOES have a perimeter, which is the pixel itself.

(I hit this when implementing a new feature for regionprops. But I will probably work around this bwperim issue in regionprops, to not create too many inter-dependencies of patches.)

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39615:  bwperim_v2.diff added by avinoam (735B - application/octet-stream)
file #39614:  bwperim.diff added by avinoam (505B - 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 avinoam (Updated the item)
  • -email is unavailable- added by hardy (Submitted the item)
  • -email is unavailable- added by hardy (cc-ing people who care about the image package)
  • -email is unavailable- added by hardy (cc-ing people who care about the image package)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-03 carandraug StatusPatch Reviewed => Fixed
        Open/ClosedOpen => Closed
    2017-01-30 avinoam StatusConfirmed => Patch Reviewed
    2017-01-30 avinoam Attached File- => Added bwperim_v2.diff, #39615
    2017-01-30 avinoam Attached File- => Added bwperim.diff, #39614
        Summaryimage package: bwperim result on 1x1input incorrect => image package: bwperim result on 1x1 input incorrect
    2017-01-27 avinoam StatusNone => Confirmed
    2017-01-26 hardy Carbon-Copy- => Added avinoam
        Carbon-Copy- => Added carandraug

    Back to the top


    Powered by Savane 3.5