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
|
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.
|
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!
|
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.
|
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!
|
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)
|
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?
|
Mon 30 Jan 2017 07:11:31 PM UTC, comment #3:
The fix from comment #1 attached as a patch.
(file #39614)
|
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.)
|
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:
|
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.)
|