bugGNU Octave - Bugs: bug #67031, (image) std2 needs to be adapted...

 
 

bug #67031: (image) std2 needs to be adapted for uint data

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Wed 16 Apr 2025 05:28:55 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  avinoam
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Post a Comment

Add a New Comment Rich Markup
   

Discussion

Jump to the original submission

Sun 04 May 2025 04:33:54 AM UTC, comment #14: 

Thank you all for your teamwork.

The updated function is now incorporated into the newly released version (2.16.1) of the image package.

Closing as fixed.

Avinoam Kalma <avinoam>
Group Member
Fri 02 May 2025 04:37:55 AM UTC, comment #13: 

Thanks! Looks fine now:

Summary:

  PASS                             2219
  FAIL                                0
  XFAIL (expected failure)           35


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 02 May 2025 04:06:55 AM UTC, comment #12: 

Thanks for testing.

I have added eps for double-type outputs and 2*eps for single type.

https://sourcefor ... c3cc4e63d2990343/

Please check if it is enough.

Avinoam Kalma <avinoam>
Group Member
Thu 01 May 2025 02:55:29 PM UTC, comment #11: 

You may want to add some "eps":

octave:4> test std2 verbose
>>>>> /Users/dmitri/.local/share/octave/api-v60/packages/image-2.17.0+/std2.m
***** test
 a = std2 (uint16 (eye (10)));
 b = std (eye (10)(:));
 assert (a, b);
 A = eye(10) - 0.1;
 A2 = A .* A;
 s = sum (A2(:));
 res = sqrt (s / 99);
 assert (std2 (eye (10)), res);
 assert (std2 (single (eye (10))), single(res), 2e-7);
 assert (std2 (uint8 (eye (10))), res);
 assert (std2 (int8 (eye (10))), res);
 assert (std2 (uint8 (eye (10))), res);
 assert (std2 (uint16 (eye (10))), res);
 assert (std2 (int16 (eye (10))), res);
 assert (std2 (int32 (eye (10))), res);
 assert (std2 (uint32 (eye (10))), res);
 assert (std2 (int64 (eye (10))), res);
 assert (std2 (uint64 (eye (10))), res);
 assert (std2 (int64 (2^63 * eye (10))) / 2^63, res);
 assert (std2 (uint64 (2^64 * eye (10))) / 2^64, res);
 assert (class (std2 (eye (10))), 'double');
 assert (class (std2 (single (eye (10)))), 'single');
 assert (class (std2 (uint8 (eye (10)))), 'double');

!!!!! test failed
ASSERT errors for:  assert (std2 (eye (10)),res)

  Location  |  Observed  |  Expected  |  Reason
     ()        0.30151      0.30151      Abs err 5.5511e-17 exceeds tol 0 by 6e-17


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 28 Apr 2025 06:35:37 PM UTC, comment #10: 

Thank you all for the review.

Pushed
https://sourcefor ... 580dc54ef3731447/

https://sourcefor ... 03d1d9b392a36983/

https://sourcefor ... 57784e1a32af8908/

Please let me know if we need more fixes.

Avinoam Kalma <avinoam>
Group Member
Sat 26 Apr 2025 02:32:23 PM UTC, comment #9: 

Instead of always converting to class double, do it only if input is not a floating point, i.e., leave input alone if input is class single. So instead of this:


-  s = std (I(:));
+  s = std (double (I(:)));


do this (untested - also, add a test for this case):


-  s = std (I(:));
+  if (isfloat (I))
+    s = std (I(:));
+  else
+    s = std (double (I(:)));
+  endif


This is the standard in the image package, i.e., when conversion to another type is needed, always do it to double (even if single class would suffice) but only do conversion if the input type makes no sense for the operation.

Also, on the patch, <> are removed from the email address and links. Please don't do that.

---

One other tip for the future:

> i notice imread only mentions 8 and 16 bit images, so maybe anything 32+ can remain unsupported for now.


Don't assume that input for these functions will come from imread: 1) imread is the core function that reads images but people may read images with their own functions that support other types; 2) the image to be processed may have been generated and not read from a file; and 3) functions from the image package can be used with input that are not necessarily images.

Carnë Draug <carandraug>
Group Member
Fri 25 Apr 2025 04:37:46 PM UTC, comment #8: 

here's an updated patch that removes the check, adds that as a BIST, and annotates the NEWS file.

(file #57169)

Nicholas Jankowski <nrjank>
Group Member
Fri 25 Apr 2025 04:11:56 PM UTC, comment #7: 

Here is a Matlab result to make a BIST from it:


im = [2, 3+4i; 55, 66+77i];
std2(im)

ans =

   50.760056474883214


And the core Octave output for "std(im(:))" is currently also 50.760.

Hartmut <hardy>
Fri 25 Apr 2025 03:12:57 PM UTC, comment #6: 

I didn't do a matlab compatibility check on that.  I'd recommend allowing complex inputs and adding a couple BISTs that make sure we're compatible there.  I haven't looked at how matlab and octave handle complex inputs to std.

Nicholas Jankowski <nrjank>
Group Member
Fri 25 Apr 2025 03:01:30 PM UTC, comment #5: 

I have also briefly checked your patches, Avinoam and Nicholas. They look fine to me.

Just one thing, that we might want to change: The function std2 now checks for real-valued inputs. I know that the function already did this check in older versions before. But I think this check is unnecessary. Sometimes complex-valued input images are useful, and I do not see a good reason to prevent the user from using this. Couldn't we just delete the input check for "real-ness", and delete the corrosponding BIST as well? (As far as I can see, Matlab also allows for comlex input images in std2.)

Hartmut <hardy>
Fri 25 Apr 2025 04:43:21 AM UTC, comment #4: 

Thanks for the review.
I will push it

Avinoam Kalma <avinoam>
Group Member
Thu 24 Apr 2025 03:50:34 PM UTC, comment #3: 

looks good.  I added a test for the isreal error validation check. attached patch includes that and a NEWS note, played around with whitespace a bit.

this looks good to push.  if you put out a new pkg release it could catch the upcoming 10.2 release cycle.

(file #57167)

Nicholas Jankowski <nrjank>
Group Member
Sun 20 Apr 2025 03:27:22 PM UTC, comment #2: 

It seems that calling std with double(I) does the work.
Please review the attached patch

(file #57151)

Avinoam Kalma <avinoam>
Group Member
Wed 16 Apr 2025 08:22:56 PM UTC, comment #1: 

Adding package maintainers

Avinoam Kalma <avinoam>
Group Member
Wed 16 Apr 2025 05:28:55 PM UTC, original submission:  

std was recently changed to produce an error for non-floating point inputs, as previously it was allowing inputs that produced incorrect results due to integer math and/or under/overflow, as discussed over in bug #67016.  This change made std2 in the image package currently non-functional for any uint image types.

This bug report is created to track corrections to that function. The primary correction is to handle uint types without underflowing negative result calculations, ensure integer rounding/truncation isn't affecting the std calculations.  Simply converting to double before calling std (or to single if memory is a concern) should address those. 

That will potentially fail though if the package wants to be able to handle uint64 images (uncommon, but they exist) as flintmax is smaller than intmax('uint64') and intmax('int64').  for now, it could maybe just issue an error/warning if such an image is loaded. i notice imread only mentions 8 and 16 bit images, so maybe anything 32+ can remain unsupported for now.

Note that mean2 is not similarly affected, as mean has been adapted for uint and large int inputs.

e.g., ideally the following test should pass:


## avoid uint image underflow
a = std2 (uint16 (eye (10)))
b = std (eye (10)(:))
assert (a,b)



Nicholas Jankowski <nrjank>
Group Member

 

Attached Files

Attached Files
file #57169:  std2_bug67031_v2.patch added by nrjank (3.9KiB - application/octet-stream)
file #57167:  std2_bug67031.patch added by nrjank (3.8KiB - application/octet-stream)
file #57151:  std2.patch added by avinoam (2.0KiB - application/octet-stream)

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

Attach Files:
   
   
Comment:
   

 

Dependencies

This item does not depend on any other items.

No items depend on this one.

 

Mail Notification Carbon-Copy List

Carbon-Copy List
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by nrjank (Submitted the item)
  •  

    Votes

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    History

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2025-05-04 avinoam StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2025-04-28 avinoam StatusPatch Submitted Patch Reviewed
    2025-04-25 nrjank Attached File- Added std2_bug67031_v2.patch, #57169
    2025-04-24 nrjank Attached File- Added std2_bug67031.patch, #57167
    2025-04-20 avinoam Attached File- Added std2.patch, #57151
        StatusConfirmed Patch Submitted
    2025-04-16 avinoam Assigned toNone avinoam
        Carbon-Copy- Added hardy
        Carbon-Copy- Added carandraug

    Back to the top

    Powered by Savane 3.16-11ef.
    Corresponding source code