[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Octave-patch-tracker] [patch #8205] Addition to financial package: blk*
From: |
Carnë Draug |
Subject: |
[Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions |
Date: |
Tue, 08 Oct 2013 20:36:58 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130917 Firefox/17.0 Iceweasel/17.0.9 |
Update of patch #8205 (project octave):
Status: None => Done
_______________________________________________________
Follow-up Comment #1:
I have pushed your changes after editing the commit message (following the
commit message guidelines[1] but still keeping your signature) and adding the
new functions to the INDEX and NEWS files [2].
I have then made a few more changes[3] to follow Octave code guidelines:
0 use endif, endfunction, etc to end blocks rather than a general end;
0 replaced tabs per spaces;
0 moved the one sentence help text to the bottom (so that lookfor can be used
to find this functions);
0 used double quotes for strings;
0 always leaving a spaces after a function call (so we can index between
function call and variable indexing);
0 added a minimum input check (just checking the number of arguments);
0 set the variable defaults on the function definition line, rather than
checking if the variable name exists
0 if cheking if a condition is false, use "if (! condition)" and not "if
(condition == 0)"
On the subject of input checking would be great if you could extend it.
On blsimpv you use two for loops. This is likely to be slow. In addition, it
will only work up to 2 dimensions. And since Octave is column-major order, I
have swapped the order of it so it will process down each column.
Finally, could you write some tests for these functions? See for example, the
test blocks at the bottom of addtodate[4], datenum[5], or rgb2ntsc[5].
[1] http://wiki.octave.org/Commit_message_guidelines
[2]
http://sourceforge.net/p/octave/financial/ci/bb3ab896bc3319907dabc7e05d25164c2b17e76a/
[3]
http://sourceforge.net/p/octave/financial/ci/7350d2b4eaab53a26cfd933f19a5561a439c1f83/
[4]
http://hg.savannah.gnu.org/hgweb/octave/file/b43da3876b64/scripts/time/addtodate.m
[5]
http://hg.savannah.gnu.org/hgweb/octave/file/b43da3876b64/scripts/time/datenum.m
[6]
http://hg.savannah.gnu.org/hgweb/octave/file/b43da3876b64/scripts/image/rgb2ntsc.m
_______________________________________________________
Reply to this item at:
<http://savannah.gnu.org/patch/?8205>
_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions, Parsiad Azimzadeh, 2013/10/07
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions,
Carnë Draug <=
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions, Parsiad Azimzadeh, 2013/10/09
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions, Parsiad Azimzadeh, 2013/10/10
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions, Parsiad Azimzadeh, 2013/10/10
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions, Carnë Draug, 2013/10/17
- [Octave-patch-tracker] [patch #8205] Addition to financial package: blk*, bls*, and opprofit functions, Carnë Draug, 2013/10/25