patchGNU Octave - Patches: patch #9409, [octave forge] (signal) modulate:...

 
 

patch #9409: [octave forge] (signal) modulate: new function, partial implementation

Submitter:  None
Submitted:  Sun 16 Jul 2017 02:44:11 PM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Postponed Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 11 Oct 2018 08:48:57 PM UTC, comment #7: 

Also, it seems to me that it is better to write


t = ((0:len-1) / fs)';


instead of


t = (0: 1/fs: ((len-1)/fs))';


or is there a specific reason that you chose the latter form?

John W. Eaton <jwe>
Group administrator
Thu 11 Oct 2018 08:43:46 PM UTC, comment #6: 

Sudeepam, the function looks good.  I just have a few comments:

There is no need to include specific links to mathworks.com.

We don't normally captitalize error messages.  Write error ("modulate: invalid range..."); not error ("modulate: Invalid Range...");

I think this function should generalize to N-dimensional arrays and simply work down the columns of all dimensions.  When looking for overall min/max values, write "min (x(:))" instead of "min (min (x))" and it will work for any number of dimensions.

You can write


function [y, t_out] = modulate (x, fc, fs, method = "am", opt)


to set the default value of the method parameter instead of checking nargin for that one.


John W. Eaton <jwe>
Group administrator
Wed 10 Oct 2018 05:30:35 PM UTC, comment #5: 

That's fine with me, I'm in no rush. The vco function in patch #9698 may be rewritten to depend on your modulate function.

Mike Miller <mtmiller>
Group Member
Wed 10 Oct 2018 05:10:29 PM UTC, comment #4: 

No problem Mike. However, I would need some time because I've been a little busy these days. I'll re-check and clean the implementation, write some tests, and add another file here by the end of the first week of November or so. Will that be okay?

P Sudeepam <sudeepam>
Wed 10 Oct 2018 04:49:12 PM UTC, comment #3: 

Hi Sudeepam, I haven't had a chance to review or test this yet in detail, but I do notice that there are no test cases. Would you mind adding some simple tests and/or demos to the end of the function? The indentation in the function also needs to be cleaned up.

Mike Miller <mtmiller>
Group Member
Wed 18 Apr 2018 06:05:16 PM UTC, comment #2: 

Here is an independent rewrite with all the modulation schemes and it mostly follows the coding standards. However, I've included some additional comments in the code (with a ###) that may help during the review but may not be necessary otherwise. Also, I am only attaching a .m file as of now and will follow a .patch file after the initial stages of review (if that is fine). Kindly review the patch and inform me of all the changes that are required.

(file #43969)

P Sudeepam <sudeepam>
Fri 30 Mar 2018 02:00:49 AM UTC, comment #1: 

Does not include ppm and pwm options as of now.

(Copied text from original submission summary line)

Mike Miller <mtmiller>
Group Member
Sun 16 Jul 2017 02:44:11 PM UTC, original submission:  


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43969:  modulate.m added by sudeepam (7KiB - text/x-objcsrc - rewrite with all the modulation schemes. -P Sudeepam)
file #41212:  modulate.m added by None (2KiB - application/octet-stream - Added modulate ..still missing ppm and pwm)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by sudeepam (Updated the item)
  • -email is unavailable- added by None (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 logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-10 mtmiller Carbon-CopyRemoved 80942 -
    2018-04-18 sudeepam Attached File- Added modulate.m, #43969
    2018-03-30 mtmiller StatusNone Postponed
        SummaryImplemented modulate function for Forge Signal package however it does not include ppm and pwm options as of now. [octave forge] (signal) modulate: new function, partial implementation
    2017-07-16 None Attached File- Added modulate.m, #41212

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code