[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Octave-patch-tracker] [patch #8575] [octave forge] (signal) lpc: new fu
From: |
Mike Miller |
Subject: |
[Octave-patch-tracker] [patch #8575] [octave forge] (signal) lpc: new function |
Date: |
Tue, 10 Mar 2020 18:28:53 -0400 (EDT) |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.132 Safari/537.36 |
Follow-up Comment #8, patch #8575 (project octave):
Keep in mind that the signal package, like most Octave Forge packages, is
community maintained, anyone is welcome to review patches and contributions.
But we are all volunteers and do what we can, and so far I am the only one who
has commented here.
I spent about 15 minutes and took a quick look at the latest submission here,
and it seems mostly right, but I can see a few improvements that I would like
to have made before committing it.
1. All strings should be double quoted, except where excessive backslashes
make it unreadable. So the "xlabel" in the demo is ok, but the "xcorr" call
should use double quotes.
2. The "@seealso" markup is lost because it comes after "@end deftypefn". Use
the "@dots{}" Texinfo macro instead of a literal "..." when eliding an array
or list.
3. Please delete all trailing whitespace. You can probably configure your
editor to do this automatically, or do a global search and replace.
4. Demos shouldn't create new figures, they should use the implied figure
created by the demo function. Multiple plots should either be subplots or
should be made into separate demos.
5. In the second demo you have a long sprintf line, that should have a space
before the first paren, and the long line should be split with "..." and
string concatenation, no backslashes.
6. Instead of "[m, n] = size (...", use "n = columns (...", since the "m"
variable is never used.
7. I think you should rearrange the order of the "@deftypefn/x" lines. In
Octave, we typically start with the simplest calling form, and then show
increasingly complex syntaxes with more arguments and return values. The first
line should be the one that takes one argument and returns one value, followed
by two arguments, followed by a line like "{[@var{a}, @var{g}] =} lpc
(@dots{})" to show that either of the above can be used with two return
values. See for example the docstring of the "fopen" function.
Other than these relatively small issues, I think this is an excellent
submission, and with those changes can be added to the signal package. It
would be even better if you could take the time to use Mercurial to create a
proper changeset that can be imported to add this function and the appropriate
NEWS file entry.
I can take care of all of these things given enough time, energy, and
interest, but the more that you or anyone else can help out, the easier this
can be added to the package.
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/patch/?8575>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/