bugGNU Octave - Bugs: bug #59477, __ode15__.cc does not consider the...

 
 

bug #59477: __ode15__.cc does not consider the number of arguments in the event function

Submitter:  Hg200 <hg200>
Submitted:  Tue 17 Nov 2020 11:15:37 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Hg200 Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 20 Jan 2021 10:10:18 AM UTC, comment #8: 

The (modified) BIST passes on all buildbots afaict. And no reports about related issues since this change.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Thu 14 Jan 2021 03:08:47 PM UTC, comment #7: 

Thank you. Looks good to me.

I pushed to default here:
https://hg.savannah.gnu.org/hgweb/octave/rev/7bf91e98bfc6

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Jan 2021 11:22:37 PM UTC, comment #6: 

I cleaned the code a bit and attached the patch. The idea is that when calling the function "__ode15__()" in ode15i.m and ode15s.m, an additional parameter is passed that controls the number of arguments in the ode event callback. Thus _ode15_ can provide the required number of arguments for ode15s and ode15i functions at line 775.

The question about runtime is tricky. I tried again a bit harder: Rebooted, no X-server and ran Octave + examples below from bash. This time i got 6% for ode15s and 0.5% for ode15i (wrapper slower than C-solution). That confirms the measurement below. But if I choose another ode problem that requires much more runtime, then both solutions have nearly same runtime. So it seems also to depend on the use case.

(file #50704)

Hg200 <hg200>
Mon 11 Jan 2021 04:59:49 PM UTC, comment #5: 

@hg200: Since you seem to already have made the necessary changes for the performance comparison, could you please provide a patch?

Markus Mützel <mmuetzel>
Group administrator
Sun 06 Dec 2020 05:15:19 PM UTC, comment #4: 

Imho, 7 % difference in runtime is noteworthy. Even if we adjust for the 2 % uncertainty from the experiment with "ode15i" and assume that a similar measurement error occurred in the experiment with "ode15s", the difference is still significant.

I'd vote for adapting the C++ code.

Markus Mützel <mmuetzel>
Group administrator
Wed 18 Nov 2020 10:23:19 PM UTC, comment #3: 

Thank you very much for the feedback. Your solution works very well. I like it because it is less code-invasive than making the change in the C code. The question about the runtime is a bit difficult to answer because it depends on how often the event function is called (i.e. how many steps have to be calculated) compared to the initialization overhead. To get some numbers I made a tic / toc measurement over five consecutive program calls. The used programs are attached. I have also attached the changes I made in __ode15__.cc for the measurement. This should be what you suggest in comment #2.

ode15s()
--------
i.) 2 to 3 wrapper in ode15s.m (odf15s-events-diff.txt)
6.31828+6.35755+6.25858+6.40452+6.29574

ii.) Adjust number of event function parameters in __ode15__.cc (events-c-solution-diff.txt)
5.80774+5.84905+5.73096+5.90781+6.24962

Conclusion: Wrapper 7% slower than C solution.

ode15i()
--------
i.) 2 to 3 wrapper in ode15s.m (has no effect on ode15i)
5.63038+5.9534+5.66035+5.75075+5.6973

ii.) Adjust number of event function parameters in __ode15__.cc (events-c-solution-diff.txt)
5.53875+5.62682+5.56293+5.62556+5.66299

Conclusion: Wrapper 2% slower than C solution (wrapper should not add overhead in ode15i).

In my opinion both solutions are ok. Assuming that the measurement is ok, I tend a little bit more to the simplicity at the expense of the runtime.

(file #50297, file #50298, file #50299)

Hg200 <hg200>
Wed 18 Nov 2020 03:11:14 PM UTC, comment #2: 

The attached patch should work, but at the cost of an additional function handle evaluation for each call to the events function.  If that is too much, then I think the better solution would be to change _ode15_ to call the events function with the correct number of inputs depending on whether it is called for ode15i or ode15s.  If we go that direction, I would do it by adding a parameter or option to _ode15_, not by examining the call stack.

(file #50296)

John W. Eaton <jwe>
Group administrator
Wed 18 Nov 2020 03:02:57 AM UTC, comment #1: 

If the event function can be a handle that refers to a MEX or .oct file function, then I don't think we can always have a way to ask it how many arguments it can accept.  So it seems _ode15_ needs to know whether it is being called to do the work of ode15i or ode15s so it can make the proper call, or ode15s needs to do something like


options.Events = @(t, y, ~) options.Events (t, y);


to place a wrapper that can accept 3 arguments but only passes 2 of them to the event function provided by the user.

John W. Eaton <jwe>
Group administrator
Tue 17 Nov 2020 11:15:37 PM UTC, original submission:  

This is an interface problem in the function ode15s(), which manifests since cset 28562:b0b80efecea1 "error for functions called with too many inputs or outputs". The attached minimal example for ode15s() runs fine with Matlab R2018 and also with Octave 6.0.9x, but it fails on the development branch. The reason for the error is the following:

According to Matlab and its documentation the event function of ode15s() has two input parameters:


ode_events (t, y)


But the event function of ode15i() has three input parameters:


ode_events (t, y, yp)


ode15s() and ode15i() are both implemented in the file __ode15__.cc. In __ode15__.cc around line 766 (IDA::event), however, the event function is evaluated with three input parameters:


octave_value_list args = ovl (tsol, y, yp);
...
octave_value_list output = feval (event_fcn, args, 3);


In the case of ode15s(), however, the event callback has only two parameters. Therefore you get following error when running the attached example on default branch:


error: ode_events: function called with too many inputs
error: called from
    ode15sfun>ode_events
    ode15s at line 315 column 22
    ode15sfun at line 14 column 22


The relation to cset b0b80efecea1 is, that before b0b80efecea1 the number of arguments was not checked when calling a function. But now they are checked and if they do not match, the error is thrown. Furthermore b0b80efecea1 is not merged to stable and therefore this problem does not yet manifest on stable.

Fixing ideas: If we want to maintain the use of "yp" in ode15s() we need to determine the number of input arguments of "event_fcn" and based on this we could compute different sized "args" in IDA::event. In pseudo code something like this:


if (nargin(@event_fcn) == 2)
  octave_value_list args = ovl (tsol, y);
else
  octave_value_list args = ovl (tsol, y, yp);
...
octave_value_list output = feval (event_fcn, args, 3);


Typically in Octave C code the number of input arguments "nargin" of a function is taken from the argument list "args.length ()". But we only have the function handle. Is it possible to determine the number of input arguments also from a pure function handle?

Other ideas to fix this issue?

Hg200 <hg200>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50704:  patch_ode15_eventfct.diff added by hg200 (10KiB - text/x-patch)
file #50298:  ode15i_runtime.m added by hg200 (1KiB - text/x-objcsrc)
file #50299:  ode15s_runtime.m added by hg200 (829B - text/x-objcsrc)
file #50296:  odf15s-events-diff.txt added by jwe (2KiB - text/plain)
file #50293:  ode15sfun.m added by hg200 (612B - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by hg200 (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-20 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-01-14 mmuetzel StatusNone Ready For Test
    2021-01-11 hg200 Attached File- Added patch_ode15_eventfct.diff, #50704
    2020-11-18 hg200 Attached File- Added events-c-solution-diff.txt, #50297
        Attached File- Added ode15i_runtime.m, #50298
        Attached File- Added ode15s_runtime.m, #50299
    2020-11-18 jwe Attached File- Added odf15s-events-diff.txt, #50296
    2020-11-17 hg200 Attached File- Added ode15sfun.m, #50293

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code