bugGNU Octave - Bugs: bug #65668, test suite and demos reset 'tic'

 
 

bug #65668: test suite and demos reset 'tic'

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Wed 01 May 2024 09:02:49 PM UTC
   
 
Category:  Test Suite Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 9.1.0 Operating System:  * Any
Fixed Release:  10.1.0 (current default) Planned Release:  10.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 03 May 2024 06:12:30 PM UTC, comment #4: 

Looks good.  Marking bug as Fixed and closing report.

Rik <rik5>
Group administrator
Fri 03 May 2024 06:02:45 AM UTC, comment #3: 

realized i had this pushed against stable not default. backed that out and pushed to default instead as:
https://hg.savannah.gnu.org/hgweb/octave/rev/57bbf1e9b4de


Nicholas Jankowski <nrjank>
Group Member
Fri 03 May 2024 05:45:09 AM UTC, comment #2: 

pushed just the ode23s demo tic/toc changes as:
https://hg.savannah.gnu.org/hgweb/octave/rev/54aa0f0a89db

as is, it looks like the test within system.tst tests the no-variable assignment form and and a separate test in data.cc tests the with-variable assignment form for tic/toc. not certain why they aren't together, but agree it's probably best to keep both even with the global timer reset.  as such, can probably just call this good enough. marking as ready for test.

Nicholas Jankowski <nrjank>
Group Member
Wed 01 May 2024 09:34:17 PM UTC, comment #1: 

I don't think there's any problem with modifying ode23.m.  I might suggest using variable name "tmr" for "timer" instead of 'T'.  I think it's a little clearer and jwe hates uppercase variable names unless they are constants.

For system.tst, I'm not sure it is a good idea to change.  I took a look at the C++ code for toc() and if we shift everything to using a timer ID then there is a code path that will be untested.  Also, while it is desirable for '__run_test_suite__' not to modify anything in the environment, it isn't a requirement.  This is not a function one would call in an ordinary Octave session.

Rik <rik5>
Group administrator
Wed 01 May 2024 09:02:49 PM UTC, original submission:  

while running checks on the test suite it was determined that tic/toc doesn't work for overall timing because a tic is called resetting the timer. this occurs within system.tst:

%!test
%! tic ();
%! pause (2);
%! assert (toc > 0);

a quick search showed this also occurs within the first few demos for ode23s:

%!demo
%! ## Demo function: stiff Van Der Pol equation
%! fcn = @(t,y) [y(2); 10*(1-y(1)^2)*y(2)-y(1)];
%! ## Calling ode23s method
%! tic ()
%! [vt, vy] = ode23s (fcn, [0 20], [2 0]);
%! toc ()
%! ## Plotting the result
%! plot (vt,vy(:,1),'-o');

%!demo
%! ## Demo function: stiff Van Der Pol equation
%! fcn = @(t,y) [y(2); 10*(1-y(1)^2)*y(2)-y(1)];
%! ## Calling ode23s method
%! odeopts = odeset ("Jacobian", @(t,y) [0 1; -20*y(1)*y(2)-1, 10*(1-y(1)^2)],
%!                   "InitialStep", 1e-3)
%! tic ()
%! [vt, vy] = ode23s (fcn, [0 20], [2 0], odeopts);
%! toc ()
%! ## Plotting the result
%! plot (vt,vy(:,1),'-o');

%!demo
%! ## Demo function: stiff Van Der Pol equation
%! fcn = @(t,y) [y(2); 100*(1-y(1)^2)*y(2)-y(1)];
%! ## Calling ode23s method
%! odeopts = odeset ("InitialStep", 1e-4);
%! tic ()
%! [vt, vy] = ode23s (fcn, [0 200], [2 0]);
%! toc ()
%! ## Plotting the result
%! plot (vt,vy(:,1),'-o');

%!demo
%! ## Demo function: stiff Van Der Pol equation
%! fcn = @(t,y) [y(2); 100*(1-y(1)^2)*y(2)-y(1)];
%! ## Calling ode23s method
%! odeopts = odeset ("Jacobian", @(t,y) [0 1; -200*y(1)*y(2)-1, 100*(1-y(1)^2)],
%!                   "InitialStep", 1e-4);
%! tic ()
%! [vt, vy] = ode23s (fcn, [0 200], [2 0], odeopts);
%! toc ()
%! ## Plotting the result
%! plot (vt,vy(:,1),'-o');

if these are changed from `tic()` to t = tic(), then the calls would not reset the global tic timer.  The attached patch makes that change in both files.  I don't see any issues with making this change in the demos, but is there any concern about testing a variable assignment tic/toc vs the current no-argument tic/toc in system.tst? I noticed test coverage doesn't include both forms, is there a particular need to test both?

Nicholas Jankowski <nrjank>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55995:  bug65668testdemotics.patch added by nrjank (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-05-03 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 10.1.0 (current default)
    2024-05-03 nrjank StatusPatch Reviewed Ready For Test
    2024-05-02 rik5 Item GroupIncorrect Result Other
        StatusNone Patch Reviewed
        Planned ReleaseNone 10.1.0 (current default)
    2024-05-01 nrjank Attached File- Added bug65668testdemotics.patch, #55995

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code