bugGNU Octave - Bugs: bug #53435, tests: camorbit.m, camroll.m: two...

 
 

bug #53435: tests: camorbit.m, camroll.m: two failing tests on i686

Submitter:  Mike Miller <mtmiller>
Submitted:  Fri 23 Mar 2018 06:34:33 AM UTC
   
 
Category:  Test Suite Severity:  3 - Normal
Priority:  1 - Later Item Group:  Inaccurate Result
Status:  Postponed Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 09 Apr 2018 01:39:54 AM UTC, comment #15: 

@Markus: I checked in your cset for camroll here http://hg.savannah.gnu.org/hgweb/octave/rev/f83c2015cbc7.

Rik <rik5>
Group administrator
Sun 08 Apr 2018 01:21:38 PM UTC, comment #14: 

There was an error in camroll (and apparently in earlier Matlab versions) that the dataaspectratio was not considered. The attached patch should fix that error and allows the tests to pass with the updated reference values from recent Matlab versions.

It should be save to apply this patch on stable because no big changes are made (and it fixes a bug).

That still leaves the original issue why the tests need broader tolerances on 32bit architectures. But that definitely has time for after 4.4 is released.

(file #43840)

Markus Mützel <mmuetzel>
Group administrator
Sun 08 Apr 2018 08:46:58 AM UTC, comment #13: 

@Dmitri and Tik: That's what I tried to point out in comment #8. Something seems to have changed between Matlab versions that changed how camroll calculates the CameraUpVector. In the meantime, Himanshu Bhadani confirmed on the mailing list that he sees the same (new) results with Matlab R2017b. [1]

The changeset with the updated reference values isn't really needed for the 4.4 release. We could either revert in on stable (and only apply on default). Or we could mark the failing test as a known bug.
I didn't have time yet to check whether there is an easy fix. But I will try and check today.

[1]: http://octave.1599824.n4.nabble.com/Matlab-test-for-camup-td4687417.html

Markus Mützel <mmuetzel>
Group administrator
Sat 07 Apr 2018 11:35:32 PM UTC, comment #12: 

But error is just a way off...
I thought meya be there is a typo somewhere.

Dmitri.

Dmitri A. Sergatskov <dasergatskov>
Sat 07 Apr 2018 11:32:33 PM UTC, comment #11: 

Maybe the patch with updated values also returned to tighter tolerances which had been loosened by Mike in order to get everything to pass?

Rik <rik5>
Group administrator
Sat 07 Apr 2018 11:22:19 PM UTC, comment #10: 

I just saw on stable buildbot:

http://buildbot.octave.org:8010/builders/stable-gcc-fedora/builds/6/steps/test/logs/stdio


>>>>> processing /home/buildbotu/fc25-x86_64/stable-gcc-fedora/src/scripts/plot/appearance/camroll.m
***** test
 hf = figure ("visible", "off");
 unwind_protect
   peaks ();
   p = camup ();
   assert (p, [0 0 1], eps);
   camroll (30);
   p = camup ();
   ## from Matlab R2014a
   q = [0.826398839602911  0.255644120004753  0.50170812412194];
   ## from Matlab R2016a
   q = [0.33969638129660373 0.02014238382998192 0.94031944194919104];
   assert (p, q, 10*eps);
   camroll (-30);
   ## note it does not go back to [0 0 1]: instead orthog to camera view:
   p = camup ();
   ## The following assert doesn't seem to have any validation in Matlab R2016a
   assert (dot (p, camtarget () - campos ()), 0, 32*eps);  # FIXME: looser tolerance needed on i386
   ## from Matlab R2014a
   q = [0.496200420425837  0.646660977913424  0.57932264103285];
   ## from Matlab R2016a
   q = [0.14033891839365262 0.18289323924769943 0.97306477226420207];
   assert (p, q, 10*eps);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect
!!!!! test failed
ASSERT errors for:  assert (p,q,10 * eps)

  Location  |  Observed  |  Expected  |  Reason
    (1)         0.8264       0.3397      Abs err 0.4867 exceeds tol 2.2204e-15 by 0.5
    (2)        0.25564      0.020142     Abs err 0.2355 exceeds tol 2.2204e-15 by 0.2
    (3)        0.50171      0.94032      Abs err 0.43861 exceeds tol 2.2204e-15 by 0.4


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sat 07 Apr 2018 10:42:34 PM UTC, comment #9: 

I pushed the updated tolerances to the stable branche here (http://hg.savannah.gnu.org/hgweb/octave/rev/c578827b2eaf).

Rik <rik5>
Group administrator
Tue 03 Apr 2018 04:53:06 PM UTC, comment #8: 

The attached patch updates the references to the highest possible precision as seen in Matlab R2016a.

Most changes only add a few more decimals. But the test in camroll.m gives very different results from the previous ones (R2014a):

## test rolling, then rolling back
%!test
%! hf = figure ("visible", "off");
%! unwind_protect
%!   peaks ();
%!   p = camup ();
%!   assert (p, [0 0 1], eps);
%!   camroll (30);
%!   p = camup ();
%!   ## from Matlab R2014a
%!   q = [0.826398839602911  0.255644120004753  0.50170812412194];
%!   ## from Matlab R2016a
%!   q = [0.33969638129660373 0.02014238382998192 0.94031944194919104];
%!   assert (p, q, 10*eps);
%!   camroll (-30);
%!   ## note it does not go back to [0 0 1]: instead orthog to camera view:
%!   p = camup ();
%!   ## The following assert doesn't seem to have any validation in Matlab R2016a
%!   assert (dot (p, camtarget () - campos ()), 0, 32*eps);  # FIXME: looser tolerance needed on i386
%!   ## from Matlab R2014a
%!   q = [0.496200420425837  0.646660977913424  0.57932264103285];
%!   ## from Matlab R2016a
%!   q = [0.14033891839365262 0.18289323924769943 0.97306477226420207];
%!   assert (p, q, 10*eps);
%! unwind_protect_cleanup
%!   close (hf);
%! end_unwind_protect


The on-screen results in R2016a seem to be reasonable. Maybe someone with a newer Matlab version might be able to check whether this is only an error in Matlab R2016a on Windows 10 64bit.

Code to execute:

hf = figure;
peaks ();
p = camup ();
fprintf ('%.17f\n', p);
camroll (30);
p = camup ();
fprintf ('%.17f\n', p);
camroll (-30);
p = camup ();
fprintf ('%.17f\n', p);



(file #43783)

Markus Mützel <mmuetzel>
Group administrator
Mon 26 Mar 2018 08:39:51 PM UTC, comment #7: 

I pushed the test change on the stable branch

https://hg.savannah.gnu.org/hgweb/octave/rev/a95140167457

Leaving this bug report open for investigation of why these tolerance increases are necessary.

Mike Miller <mtmiller>
Group Member
Sat 24 Mar 2018 03:14:11 PM UTC, comment #6: 

@Mike: I won't be able to check in Matlab probably for a week or two. But go ahead and push your patch.
I'll try and get results with more significant digits later. But as I understand our GL based graphics are single precision anyway (and the differences are very minor). Thus, relaxing the tolerance is totally fine.

I am guessing the underlying issue is that some trigonometric functions give slightly different results on different platforms. I believe there is a bug report about this somewhere. But I cannot find it right now.

Markus Mützel <mmuetzel>
Group administrator
Fri 23 Mar 2018 09:57:38 PM UTC, comment #5: 

Here is the patch that loosens the tolerances enough to pass for my testing. I will keep this pending for now and won't push this until I hear back, but this should be addressed for the 4.4 release.

(file #43654)

Mike Miller <mtmiller>
Group Member
Fri 23 Mar 2018 05:03:19 PM UTC, comment #4: 

Markus, do you have access to Matlab to get updated constants for the tests? Besides these test failures, I see explicit floating point constants from Matlab in tests for camlookat.m, camorbit.m, camroll.m, camzoom.m, and camlight.m. Matlab doesn't have printf, so that would be sprintf.

If you think this will take a while to investigate or don't have time now, I will just increase the tolerances for now and leave this report open, as with bug #53438.

Mike Miller <mtmiller>
Group Member
Fri 23 Mar 2018 03:42:29 PM UTC, comment #3: 

Note that IEEE-854 doubles have 15-17 significant digits when expressed in base 10.  The constants have only 15 significant digits.  The first step should be to update the precision of the constants used in comparison to be 17 digits.  This is longer than what is typically displayed with "format long" so you need to use printf with a format.  For example,


printf ('%.17f\n', x)



Rik <rik5>
Group administrator
Fri 23 Mar 2018 07:29:13 AM UTC, comment #2: 

I'd be happy to test further if you can tell me what to run.

Here's the output from running the minimal commands directly to show the camorbit results that are different:


>> hf = figure ('visible', 'off');
>> sphere ();
>> camorbit (20, 30, 'data', [1, 2, 3]);
>> p = get (gca, 'cameraposition');
>> format long
>> p.'
ans =

  -2.157726725251017e-01
  -9.049266154288148e+00
   1.476699780668523e+01


Mike Miller <mtmiller>
Group Member
Fri 23 Mar 2018 07:07:05 AM UTC, comment #1: 

Increasing the tolerances for these tests (on stable) should be fine.
It might make sense to track down which underlying operation is causing the deviation and whether that is a known problem.

Markus Mützel <mmuetzel>
Group administrator
Fri 23 Mar 2018 06:34:33 AM UTC, original submission:  

In testing Octave built for i686, I am seeing 1 test failure in camorbit.m and 1 test failure in camroll.m.

Both test failures look like candidates for increasing the tolerances, if someone familiar with these functions can verify that this makes sense.


 hf = figure ("visible", "off");
 unwind_protect
   sphere ();
   camorbit(20, 30, "data", [1 2 3]);
   p = campos ();
   u = camup ();
   ## Matlab 2014a
   pm = [-0.215772672525099  -9.04926615428815  14.7669978066852];
   um = [0.413058199972826  0.773801198226611  0.48022351989284];
   assert (p, pm, -5e-15);
   assert (u, um, -5e-15);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect
!!!!! test failed
ASSERT errors for:  assert (p,pm,-5e-15)

  Location  |  Observed  |  Expected  |  Reason
    (1)        -0.21577     -0.21577     Rel err 1.2477e-14 exceeds tol 5e-15 by 7e-15


 hf = figure ("visible", "off");
 unwind_protect
   peaks ();
   p = camup ();
   assert (p, [0 0 1], eps);
   camroll (30);
   p = camup ();
   ## from Matlab R2014a
   q = [0.826398839602911  0.255644120004753  0.50170812412194];
   assert (p, q, 10*eps);
   camroll (-30);
   ## note it does not go back to [0 0 1]: instead orthog to camera view:
   p = camup ();
   assert (dot (p, camtarget () - campos ()), 0, eps);
   q = [0.496200420425837  0.646660977913424  0.57932264103285];
   assert (p, q, 10*eps);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect
!!!!! test failed
ASSERT errors for:  assert (dot (p, camtarget () - campos ()),0,eps)

  Location  |  Observed  |  Expected  |  Reason
     ()      -4.3993e-15       0         Abs err 4.3993e-15 exceeds tol 2.2204e-16 by 4e-15


Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43840:  bug53435_camroll.patch added by mmuetzel (3KiB - application/octet-stream)
file #43783:  bug53435_cam_reference.patch added by mmuetzel (7KiB - application/octet-stream)
file #43654:  bug53435.diff added by mtmiller (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by mtmiller (Submitted the item)
  •  

    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.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-08 mmuetzel Attached File- Added bug53435_camroll.patch, #43840
    2018-04-03 mmuetzel Attached File- Added bug53435_cam_reference.patch, #43783
    2018-03-26 mtmiller Priority5 - Normal 1 - Later
        StatusPatch Submitted Postponed
    2018-03-23 mtmiller Attached File- Added bug53435.diff, #43654
        StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code