bugGNU Octave - Bugs: bug #65221, movfun: create inputParser only...

 
 

bug #65221: movfun: create inputParser only once.

Submitter:  A.R. Burgers <arb>
Submitted:  Sat 27 Jan 2024 09:36:30 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Performance
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  10.1.0 (current stable) Planned Release:  10.1.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 03 Jul 2024 06:45:26 AM UTC, comment #7: 

Thank you for testing.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Mon 01 Jul 2024 02:00:15 PM UTC, comment #6: 

nevermind,  I see now all other parameters using anonymous fns depend only on the primary function variable or internal hardcoded constants, so they shouldn't run into the same issue.  I can't seem to break the modified version, and all tests pass.  we can probably mark this as fixed again.


Nicholas Jankowski <nrjank>
Group Member
Mon 01 Jul 2024 01:22:25 PM UTC, comment #5: 

resetting the parser like that seems to fix it. I'm not that familiar with inputParser parameters. would there be a need to do similarly with any of the other parameters?

Nicholas Jankowski <nrjank>
Group Member
Mon 01 Jul 2024 07:35:22 AM UTC, comment #4: 

I pushed the following change that should avoid the issue (and potentially reduce the memory footprint of the function):
https://hg.savannah.gnu.org/hgweb/octave/rev/5e56c1811c32

Marking as ready for test again.

Markus Mützel <mmuetzel>
Group administrator
Mon 01 Jul 2024 01:58:58 AM UTC, comment #3: 

it seems that this fix runs into problems when movfun is called more if dim changes to something that would fail the inputParser check the first time, even if x was changed such that it should pass on the subsequent times.  E.g., calling movfun in the following fashion in the same session:


>> x = magic (4);
>> movfun(@mean, x, 2, 'dim', 1)
ans =

   16.0000    2.0000    3.0000   13.0000
   10.5000    6.5000    6.5000   10.5000
    7.0000    9.0000    8.0000   10.0000
    6.5000   10.5000   10.5000    6.5000

>> x = cat(3,x,x)
x =

ans(:,:,1) =

   16    2    3   13
    5   11   10    8
    9    7    6   12
    4   14   15    1

ans(:,:,2) =

   16    2    3   13
    5   11   10    8
    9    7    6   12
    4   14   15    1

>> movfun(@mean, x, 2, 'dim', 3)
error: movfun: failed validation of 'dim'.  Checked with "@(d) isempty (d) || (isscalar (d) && isindex (d, ndims (x)))"
error: called from
    error at line 693 column 7
    validate_arg at line 599 column 11
    parse at line 517 column 11
    movfun at line 195 column 3

>> clear all
>> clear -f
>> x = magic (4); x = cat(3, x, x)
x =

ans(:,:,1) =

   16    2    3   13
    5   11   10    8
    9    7    6   12
    4   14   15    1

ans(:,:,2) =

   16    2    3   13
    5   11   10    8
    9    7    6   12
    4   14   15    1

>> movfun(@mean, x, 2, 'dim', 3)
ans =

ans(:,:,1) =

   16    2    3   13
    5   11   10    8
    9    7    6   12
    4   14   15    1

ans(:,:,2) =

   16    2    3   13
    5   11   10    8
    9    7    6   12
    4   14   15    1


it seems that this is too persistent. reopening report.

Nicholas Jankowski <nrjank>
Group Member
Sun 30 Jun 2024 05:14:22 PM UTC, comment #2: 

See also the test regression in bug #65927 that is probably caused by this change. (Found this bug number only after writing in the other report.)

Markus Mützel <mmuetzel>
Group administrator
Sun 28 Jan 2024 11:33:26 PM UTC, comment #1: 

Good catch.  I wrote a small test script to benchmark performance.


N = 100;

x = [1:3e4];

y = movmean (x, 20, 'includenan');

tic
for i = 1:N
  y = movmean (x, 20, 'includenan');
endfor
toc


Current code: 0.86 seconds
Patched code: 0.80 seconds
% change: -6%

  The change isn't large.  Nevertheless, it is an easy win.  I checked in your changeset here https://hg.savannah.gnu.org/hgweb/octave/rev/4df9460ab8a9.  I made two tweaks to follow Octave coding guidelines: 1) use space between 'if' and condition wrapped in parentheses, 2) use specific end keyword, in this case 'endif', instead of generic 'end'.

Marking bug as Fixed and closing report.





Rik <rik5>
Group administrator
Sat 27 Jan 2024 09:36:30 PM UTC, original submission:  

movfun currently recreates an inputParser instance on every call. I think one time suffices as in scripts/ode/odeset.m.

See the attached patch.

passes test movfun.

A.R. Burgers <arb>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55627:  movfun.patch added by arb (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 nrjank (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by arb (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-03 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2024-07-01 mmuetzel StatusIn Progress Ready For Test
        Planned ReleaseNone 10.1.0 (current stable)
    2024-07-01 nrjank StatusFixed In Progress
        Open/ClosedClosed Open
    2024-01-28 rik5 StatusNone Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 10.1.0 (current stable)
    2024-01-27 arb Attached File- Added movfun.patch, #55627

    Back to the top

    Powered by Savane 3.14-9aa3.
    Corresponding source code