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 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 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 03 Apr 2017 12:50:20 PM UTC, 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 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:

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

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 03 Apr 2017 12:50:20 PM UTCcarandraugStatusPatch Reviewed=>Fixed
      Open/ClosedOpen=>Closed
    Mon 30 Jan 2017 09:41:40 PM UTCavinoamStatusConfirmed=>Patch Reviewed
    Mon 30 Jan 2017 08:26:26 PM UTCavinoamAttached File-=>Added bwperim_v2.diff, #39615
    Mon 30 Jan 2017 07:11:31 PM UTCavinoamAttached File-=>Added bwperim.diff, #39614
      Summaryimage package: bwperim result on 1x1input incorrect=>image package: bwperim result on 1x1 input incorrect
    Fri 27 Jan 2017 08:38:56 AM UTCavinoamStatusNone=>Confirmed
    Thu 26 Jan 2017 12:50:57 PM UTChardyCarbon-Copy-=>Added avinoam
      Carbon-Copy-=>Added carandraug

    Back to the top


    Powered by Savane 3.1-cleanup1