bugGNU Octave - Bugs: bug #60465, [octave forge] (signal) findpeaks...

 
 

bug #60465: [octave forge] (signal) findpeaks should plot data

Submitter:  None
Submitted:  Wed 28 Apr 2021 12:23:47 PM UTC
   
 
Category:  Octave Package Severity:  2 - Minor
Priority:  3 - Low Item Group:  Matlab Compatibility
Status:  Confirmed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Open Release:  * dev
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Wed 12 May 2021 07:12:12 PM UTC, comment #5: 

The function peaks.m is a small example of a function that either returns values or plots the values and returns nothing. Use 'type peaks.m' or 'edit peaks.m' to view the function file.

Mike Miller <mtmiller>
Group Member
Wed 12 May 2021 10:53:35 AM UTC, comment #4: 

Please try to avoid calling findpeaks.

  • Indented because of the else
  • Where? If common is it in the wiki? An example be more useful.



If a figure is already open

  • Cannot compare, no idea about axes


I think it would be better to combine

  • Agreed the colour was mentioned in the original post.

As stated this is a solution.



Anonymous
Tue 11 May 2021 12:31:08 AM UTC, comment #3: 

Still some room for improvement if you want to continue working on this.

  • Please try to avoid calling findpeaks recursively and re-indenting the entire file. Instead, keep the main body of the function the same, and just group and avoid the assignment to the output arguments if nargout is zero. See other plotting functions for multiple examples of this programming pattern.


  • If a figure is already open, what should findpeaks do? Should it reuse the current figure and overwrite whatever the user had already plotted there? Or should it always create a new figure and new axes object and plot there? What does Matlab do? From my limited testing, seems like Matlab plots into the current axes, respects and maintains the current hold state and line color sequence, but it does force the axis limits to be what it wants for the given data sequence.


  • I think it would be better to combine the 'plot' and 'scatter' into one 'plot' function call, and avoid the 'hold on'. It should also use the current line color, instead of [0 0 1].
Mike Miller <mtmiller>
Group Member
Thu 29 Apr 2021 09:51:58 AM UTC, comment #2: 

Found a solution which, solves this problem.

All code changes

if (nargout == 0)
    plot(data)
    hold on;
    [pks idx] = findpeaks(data);
    scatter(idx, pks,"v", 'MarkerFaceColor',[0 0 1], "filled")
    clear pks;
else


New patch is attached.

(file #51333)

Anonymous
Wed 28 Apr 2021 11:23:51 PM UTC, comment #1: 

Confirmed (https://www.mathworks.com/help/signal/ref/findpeaks.html for reference), thanks for the suggestion. Feel free to keep working on this and posting updated patches.

Mike Miller <mtmiller>
Group Member
Wed 28 Apr 2021 12:23:47 PM UTC, original submission:  

If findpeaks has no output it should plot the peaks and the data.

Attached is patch attempt of this.
The plotting works but the data is not suppressed.
The markers are not the same colour.

Added

  if (nargout == 0)
    plot(data)
    hold on;
    [pks idx] = findpeaks(data);
    scatter(idx, pks,"v", "filled")
  else
+verbatim+

Before
+verbatim+
if (! (isvector (data) && numel (data) >= 3))
-verbatim+

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51333:  findpeaks.patch added by None (16KiB - text/x-patch - Version 2)
file #51330:  findpeaks.patch added by None (16KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

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 6 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2021-04-29 None Attached File- Added findpeaks.patch, #51333
2021-04-28 mtmiller Severity3 - Normal 2 - Minor
    Priority5 - Normal 3 - Low
    StatusNone Confirmed
    Release4.2.2 dev
2021-04-28 None Attached File- Added findpeaks.patch, #51330

Back to the top

Powered by Savane 3.13-d3ae.
Corresponding source code