Fri 20 Nov 2015 11:17:13 AM UTC, comment #13:
> Attached changeset with tests
Thank you. I have pushed your extra tests.
|
Wed 18 Nov 2015 09:00:08 PM UTC, comment #12:
Attached changeset with tests
(file #35496)
|
Thu 12 Nov 2015 09:19:29 PM UTC, comment #11:
Pushed with cast and floor for the reasons on the comment below
http://hg.code.sf.net/p/octave/image/rev/6403613d984c
and added a simple test for int16 image
http://hg.code.sf.net/p/octave/image/rev/9d79984e9b3e
However, I would still appreciate more tests to this function.
|
Thu 12 Nov 2015 09:08:09 PM UTC, comment #10:
Well, I think it is good to make it explicit that we do want it cast and
floored (and not only cast(), remember negative values).
Interestingly, I do get the same results as you:
However, when I use random and larger matrices, that doesn't happen anymore:
For curiosity, I also timed the actual comparison step and it is faster
to make the cast explicitely:
|
Thu 12 Nov 2015 08:15:58 PM UTC, comment #9:
Thanks for fixing the cset. Please push the attached cset,
but remove me from the copyright line. Thanks :-)
Next week I hope to add more tests.
Just a question: I have compare to execution times for this
version and for version with
i.e. leaving the thresh in double, without changing image type.
using the following script
I got the same results but 20% faster.
Any reason for doing the cast & floor
|
Thu 12 Nov 2015 02:50:09 PM UTC, comment #8:
Ah, yes you are right. For some report, on the original report I read that Matlab was reporting 24 instead of 26. My changes actually made it worse. Should I push the attached cset then?
Also, since it seems you have Matlab access, I would be grateful if you could had a few more tests, with int8 and int16 input as well.
(file #35440)
|
Wed 11 Nov 2015 06:58:57 AM UTC, comment #7:
Thanks for improving the patch.
There are still two problems with the fix:
1. The addition of 1 to the range is wrong
2. ceil should be replaced by floor.
see for example:
Matlab (2010b) result is 1405, while Octave result is 1394.
We can add this small example as a BIST.
|
Tue 10 Nov 2015 07:41:55 PM UTC, comment #6:
Thank you guys for handling this.
On your last patch the computed range was off by just 1. I pushed a slightly different version (it also removes string comparison for the class name):
http://hg.code.sf.net/p/octave/image/rev/c4834a5278a4
Would be great if you could further the tests for this function, as it is, it's quite small.
|
Mon 09 Nov 2015 08:10:41 PM UTC, comment #5:
About the first point, yes you are right, a fixed patch is attached.
About the second point, if you want to make thresh in the same class as the image, you should use floor and casting, not round. If you will use round, you will get the wrong result again. But there
is no difference in the result, if the thresh is left double or being floored, so AFAIU there's no need to change the thresh from double (but maybe I'm wrong?)
(file #35428)
|
Sun 08 Nov 2015 09:23:25 PM UTC, comment #4:
In principle yes, but I still see some problems with it:
- (intmax(cls) - imin) does NOT work. Give it a try with cls='int16' on the command line. Those numbers need to be transformed for example to class double before a big values like 65535 can be hold within a variable of this type at all.
- The resulting class of thresh should NOT be double, as it is in your patch right now. But thresh needs to have the class of the input image (as it was in the original code).
Sorry, I dont' have enough time to further think about it now. I will try to come back to this at the end of the week.
|
Sun 08 Nov 2015 08:52:06 PM UTC, comment #3:
something like
??
|
Sun 08 Nov 2015 08:29:53 PM UTC, comment #2:
The provided patch looks reasonable to me for input images of the classes uint8 and uint16. But I think it will produce wrong results with int16 images because intmin('int16') = -32768. But there should be an easy way to improve this patch, by additionally using the intmin value of the input class.
|
Sun 08 Nov 2015 06:57:06 PM UTC, comment #1:
As I understand the thresh, it seems to me that if the thresh is 0.9, it should be translated to 0.9 x 255 = 229.5, without
any rounding, so 230 and up will be included, as in Matlab.
See the attached diff.
(file #35417)
|
Fri 06 Nov 2015 08:00:11 PM UTC, original submission:
I observed this behavior with Octave 4.0.0 release and the image package release 2.4.1 (under Linux). Here is a little skript to show the behavior:
If you let this skript run in Octave, the result is 25. This means the Octave version of im2bw decides that the values between 231 and 255 are above a threshold of 90%.
If you let this script run in Matlab (R2013b), the result is 26. This means the Matlab version of im2bw decides that the values between 230 and 255 are above a threshold of 90%.
This is (only) an incompatibility. The results of the Octave version of im2bw are also well usable. But they ARE different to the Matlab results, you always get a few less foreground pixels with Octave.
In my naive understanding, the Matlab result is "more correct". When I would be asked to divide the set [0 ... 255] into a chunk with 90% and another chunk with 10% of the numbers, then I would calculate 256*0.9 = 230.4. And then I would start to count 230 elements of this set, and end up with 0 ... 229. So the value 230 would be the first one of the remaining 10%, those values being 230...255. This is the Matlab result.
So I guess this issue is probably either about
- how to round a float percentage to the next integer value, or about
- starting to count at 0 or at 1, or about
- taking 255 or 256 as maximum value of the uint8 numbers.
|