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 Jan 26 12:50:57 2017  
 
Category: Octave Forge PackageSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: 4.2.0Operating System: Any

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

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

 

(Jump to the original submission Jump to the original submission)

Mon Apr 3 12:50:20 2017, comment #10:

Everyone's happy, so I pushed it http://hg.code.sf.net/p/octave/image/rev/c2a397b5bd8c

Carnë Draug <carandraug>
Project Member
Sun Apr 2 05:53:53 2017, 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 Apr 1 21:37:49 2017, 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 Mar 27 17:06:14 2017, 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 Jan 30 20:45:06 2017, 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 Jan 30 20:26:26 2017, 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 Jan 30 19:54:16 2017, 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 Jan 30 19:11:31 2017, comment #3:

The fix from comment #1 attached as a patch.

(file #39614)

Avinoam Kalma <avinoam>
Project Member
Mon Jan 30 18:13:38 2017, 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 Jan 30 17:51:19 2017, 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:

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

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

The result of this little script

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 File(s):
   
   
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
  • -unavailable- added by avinoam (Updated the item)
  • -unavailable- added by hardy (Submitted the item)
  • -unavailable- added by hardy (cc-ing people who care about the image package)
  • -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 click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon Apr 3 12:50:20 2017carandraugStatusPatch Reviewed=>Fixed
      Open/ClosedOpen=>Closed
    Mon Jan 30 21:41:40 2017avinoamStatusConfirmed=>Patch Reviewed
    Mon Jan 30 20:26:26 2017avinoamAttached File-=>Added bwperim_v2.diff, #39615
    Mon Jan 30 19:11:31 2017avinoamAttached File-=>Added bwperim.diff, #39614
      Summaryimage package: bwperim result on 1x1input incorrect=>image package: bwperim result on 1x1 input incorrect
    Fri Jan 27 08:38:56 2017avinoamStatusNone=>Confirmed
    Thu Jan 26 12:50:57 2017hardyCarbon-Copy-=>Added avinoam
      Carbon-Copy-=>Added carandraug

    Back to the top


    Powered by Savane 3.1-cleanup1