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

 
 

bug #53019: [octave forge] (signal) zplane plotting enhancements

Submitter:  Brian M. Reeder <brianr>
Submitted:  Tue 30 Jan 2018 04:57:12 PM UTC
   
 
Category:  Octave Package Severity:  2 - Minor
Priority:  3 - Low Item Group:  Inaccurate Result
Status:  Fixed Assigned to:  None
Originator Name:  Brian M. Reeder Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 08 Feb 2019 07:56:18 PM UTC, comment #9: 

I made a change to zplane.m that I think resolves this bug report. It was hard to reconcile the modified version attached here with the zplane.m in the repository, so I made roughly equivalent minimal changes to support restoring the multiplicity text labels and proper colors.

http://hg.code.sf.net/p/octave/signal/rev/768799f3e705

This will be fixed in the upcoming signal version 1.4.1.

Mike Miller <mtmiller>
Group Member
Thu 12 Apr 2018 06:38:54 PM UTC, comment #8: 

I agree with everything you are saying, but I still don't want to make any changes with respect to color until we are sure it is bringing us closer to being compatible with Matlab.

I really like a bug report to be about one specific issue. This bug report has covered three issues so far, multiplicity, color, and axis scaling. Would you do me a favor and file new individual bug reports about the color order issue and about the axis scaling issue when hold is on? This bug report can be about the multiplicity text annotation, and I can include that change right now and mark this fixed.

Mike Miller <mtmiller>
Group Member
Wed 11 Apr 2018 08:08:30 AM UTC, comment #7: 

The change would also make it consistent with the plot function. For example:
plot(0:10,0:10,2:8,0:6);
produces the same as:
plot(0:10,0:10);
hold on;
plot(2:8,0:6);

Brian M. Reeder <brianr>
Mon 09 Apr 2018 08:25:46 PM UTC, comment #6: 

As I said before I don't remember what Matlab does, but I thought calling zplane with two systems should use a different color for each system. I think that was the intention of the statement:
    col = "rgbcmy";
in the original zplane. Pzmap uses different colors when plotting multiple systems and uses the colororder property, so I changed zplane to match.

Also I thought calling zplane (and pzmap) with two systems should produce the same results as calling it separately for each system when hold is on. To extend your example:
y = 0.8 exp (1j pi * [.25 -.25]).';
z = 0.9 exp (1j pi * [.25 -.25]).';
o = 0.9  exp (1j pi * [0.2 -0.2 0.4 -0.4]).';
p = 0.98 exp (1j pi * [0.2 -0.2 0.4 -0.4]).';
zplane([y,z],[o,p]);

should produce the same as:
zplane(y,o);
hold on;
zplane(z,p);

As for the axis limits, they should respect 'Hold', but I didn't know how to implement it, (I've since found an "ishold" function). Axis scaling was in the original and similarly didn't respect 'hold', it would always scale to the last plot only. I changed it to scale to all plots rather than just the last, but I think it would be better to remove the axis scaling and let the plot autoscale, as pzmap does, but I didn't want to remove what was already there.

Brian M. Reeder <brianr>
Wed 04 Apr 2018 08:42:08 AM UTC, comment #5: 

I would like to know from someone who does have access to Matlab whether zplane actually respects the axes 'ColorOrder' and 'ColorOrderIndex' properties.

Does the following in Matlab


z = 0.9 * exp (1j * pi * [.25 -.25]).';
p = 0.98 * exp (1j * pi * [0.2 -0.2 0.4 -0.4]).';
plot (1:10);
hold on;
get (gca, 'ColorOrderIndex')
zplane (z, p);
get (gca, 'ColorOrderIndex')


return 2 and 3 respectively? And are the poles, zeros, and unit circle plotted in an orange color instead of the default blue color?

If I get a positive confirmation that zplane should use and increment the color order index, then I will be happy to integrate those changes in.

The problem with the change to query the current axis limits is that it doesn't depend on whether 'hold' is on or off.

I think a better approach might be to plot the zeros, poles, and unit circle, respecting whether 'hold' is on or off, and then adjust the limits if needed.

The change to include the multiplicity of zeros and poles looks good and does seem to be Matlab compatible since it is mentioned in the docs.

Mike Miller <mtmiller>
Group Member
Wed 21 Mar 2018 07:25:34 AM UTC, comment #4: 

The line "a = axis" ensures that a new plot in an existing object is scaled to show all plots, not just the new one. The following four lines replace the 12 in the original that scaled to show the new plot only. As I remember it, the new version is Matlab compatible, the original "zplane.m" wasn't. However it is a long time since I used Matlab.

The original plotted labels and markers and then re-plotted markers so the labels weren't visible. The new version reorders things and removes the duplicate plotting of markers.

The original used the colour order: col = "rgbcmy";
The new version uses the "colororder" property. It starts at the current "colororderindex", so a plot of two systems produces the same result as two plots of one system.

Brian M. Reeder <brianr>
Tue 20 Mar 2018 10:56:53 PM UTC, comment #3: 

This is going to take some time to review since it is not in a minimal patch format.

If you want to help me review this faster, it would be very helpful to only make the minimal changes to the function. As it is it's hard for me to tell which changes are functionally necessary and which aren't. I'm sorry if my comment #1 led you to make more changes than were necessary, I was talking about the lines that you already changed or added.

If you don't want to work on this anymore, I will try to pull out the changes from the second revision that are functionally necessary to fix this bug. I think that is everything after the "a = axis" line.

Am I right in presuming that the purpose of the "axis" addition is to allow the zplane to be plotted into an already present axes object? Is that Matlab compatible? I would rather not include that change if that's not the way Matlab works.

Mike Miller <mtmiller>
Group Member
Fri 02 Mar 2018 01:23:47 AM UTC, comment #2: 

I have modified zplane.m in accordance with octave coding style.
I have removed the bit that marked close poles / zeros as multiples as the term 'close' was arbitrary.
After spending a lot of time on it, I have not been able to create a patch file.
New version of modified zplane.m attached.


(file #43432)

Brian M. Reeder <brianr>
Wed 31 Jan 2018 10:18:32 PM UTC, comment #1: 

Thanks for your bug report. If you could please provide your changes as a patch against the original zplane.m function file. Also please try to be consistent with the Octave coding style.

The resulting plots look good to me.

Mike Miller <mtmiller>
Group Member
Tue 30 Jan 2018 04:57:12 PM UTC, original submission:  

Zplane.m

It doesn't correctly indicate multiple poles / zeros.
This is apparent in the demo, with the 'empty a' subplot only showing a single pole at the origin. It seems this has been corrected before, but the original code has not been removed and it over plots the new bit.

It doesn't use system 'colororder'

I have attached my version that:
 does correctly identify multiple poles / zeros,
 marks close poles / zeros as multiples,
 and uses the system 'colororder'

Brian M. Reeder <brianr>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43432:  zplane.m added by brianr (5KiB - text/plain)
file #43110:  zplane.m added by brianr (5KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by brianr (Submitted the item)
  • -email is unavailable- added by brianr
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-08 mtmiller StatusNeed Info Fixed
        Open/ClosedOpen Closed
    2019-02-08 mtmiller Carbon-CopyRemoved 80942 -
    2018-05-01 mtmiller Release4.2.1 dev
    2018-03-02 brianr Attached File- Added zplane.m, #43432
    2018-02-15 mtmiller Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
        Item GroupIncorrect Result Inaccurate Result
        StatusPatch Submitted Need Info
    2018-01-31 mtmiller StatusNone Patch Submitted
        Summaryzplane plotting errors [octave forge] (signal) zplane plotting enhancements
    2018-01-31 mtmiller Dependencies- bugs #53018 is dependent
    2018-01-30 brianr Attached File- Added zplane.m, #43110
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code