GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Discrete omega-damping plot and tweaks by Sup3rGeo · Pull Request #193 · python-control/python-control

Conversation

Sup3rGeo commented Feb 19, 2018

Copy link

Copy Markdown

Other tweaks:

Fixed zero markers (were invisible)
Markers for pole and zero are black
Width of y and x axis

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Victor added 4 commits

February 19, 2018 11:06


          Added discrete grid to pzmap function.

dc8410c


          Revert "Added discrete grid to pzmap function."

This reverts commit dc8410c


          Added discrete grid to pzmap function.

f8c497e

(cherry picked from commit bfd9279)


          Tweaked axis lines and zero/pole markers.

3d0bf03

(cherry picked from commit 382b4e9)

Sup3rGeo mentioned this pull request

Feb 19, 2018

Bode, Nyquist, and root locus plots are crude #49

Open

coveralls commented Feb 19, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Coverage decreased (-0.7%) to 77.142% when pulling 89dc937 on Sup3rGeo:zgrid-cherry into 601b581 on python-control:master.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sup3rGeo mentioned this pull request

Feb 22, 2018

pzmap() function not work, zero not plotted #154

Closed

murrayrm requested changes

Feb 23, 2018

View reviewed changes

murrayrm left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully checked the functionality of the PR, but there are a couple of things that should be fixed before merging.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/pzmap.py Outdated


		__all__ = ['pzmap']

		def zgrid(plt):

murrayrm Feb 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zgrid function needs a docstring + some comments in the code to describe what is going on.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sup3rGeo Feb 25, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Do you have any example I could be based on?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm Feb 25, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at nichols.py and, in particular, the nichols_grid() function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/pzmap.py Outdated

		__all__ = ['pzmap']

		def zgrid(plt):
		for zeta in linspace(0, 0.9,10):

murrayrm Feb 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The range for zeta should probably be something with a default set of parameters that can be reset by the user.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sup3rGeo Feb 25, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making zetas and wns arguments so user can override default behaviour.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/pzmap.py Outdated


		from numpy import real, imag
		from .lti import LTI
		from numpy import real, imag, linspace, pi, exp, cos, sin, sqrt

murrayrm Feb 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using numpy.pi, please use math.pi to avoid problems with sphinx. See commit aa6e0df

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sup3rGeo Feb 25, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

control/pzmap.py Outdated

		an_i = int(len(xret)/2.5)
		an_x = xret[an_i]
		an_y = yret[an_i]
		plt.annotate(str(zeta), xy=(an_x, an_y), xytext=(an_x, an_y), size=7)

murrayrm Feb 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can/should we use parameters for the size? 7 seems like a magic number.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm reviewed

Feb 24, 2018

View reviewed changes

Comment thread

control/pzmap.py Outdated

		if (Plot):
		import matplotlib.pyplot as plt

		if isdtime(sys):

murrayrm Feb 24, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be isdtime(sys, strict=True), otherwise it will be true for systems with unspecified timebases (which could be continuous or discrete).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sup3rGeo Feb 25, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are treating systems with unspecified timebases as continuous (showing s-plane omega-damping - see next comment)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm Feb 25, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that if the timebase is unspecified, it can represent either a continuous-time or discrete-time system (since for many operations it doesn't matter). Because of this, if you call isdtime on a system for a system in which timebase == None, isdtime will return true. If you want something to only happen when a system is really a discrete time system, use isdtime(sys, strict=True).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm reviewed

Feb 24, 2018

View reviewed changes

Comment thread

control/pzmap.py Outdated

		import matplotlib.pyplot as plt

		if isdtime(sys):
		zgrid(plt)

murrayrm Feb 24, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make plotting the zgrid an option, rather than the default. This will give consistency with past behavior.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Sup3rGeo Feb 25, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually having it similar to what we have on rlocus function, with an argument grid=False (even though IMHO plot is way better with the omega-damping grid)

I also realized we should also be using this on rlocus function, so I am making zgrid available for rlocus and making _sgrid_func() from rlocus available for pzmap.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

murrayrm Feb 25, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this sounds like a good approach. And if we can reuse an existing function, that is even better (although I note that _sgrid_func() is not all that well commented -:).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Refactored sgrid based on matplotlib projections and moved sgrid and …

89dc937

…zgrid to new python module.

Sup3rGeo commented Mar 11, 2018

Copy link

Copy Markdown

Author

Done some major refactoring on sgrid function based on matplotlib projections. It is not perfect though, as there is not much documentation available on using it, but it works well when moving the plot around and zooming. Please take a look at it in pzmap and rlocus plots and let me know what you think.

Currently the zgrid does not use projections so there is no update on zooming.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

abouatef commented Apr 25, 2018

Copy link

Copy Markdown

why is the #3d0bf03 commit not merged yet ? I had to manually change pzmap.py file on my machine to get the zeros to be plotted

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jwdinius commented May 13, 2018

Copy link

Copy Markdown

I am new to this repo, so forgive me this question: Why does this PR have so many commits? As this PR seems to address plotting enhancements, wouldn't it be cleaner (and easier to track) to do a rebase and squash all commits into a single "plotting enhancements" commit?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sup3rGeo commented May 14, 2018

Copy link

Copy Markdown

Author

@jwdinius just because this is my first contribution to an open-source project and no one suggested it so far :) Should I do this? I am actually waiting for a word from python-control maintainers

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm commented May 14, 2018

Copy link

Copy Markdown

Member

Either way is OK. 5 commits is not that many.

@Sup3rGeo: go ahead and squash if you want to give it a shot.

@Abubakr95: I'm still waiting to find some time to review the changes and merge.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm added this to the 0.8.1 milestone

Jun 30, 2018

murrayrm modified the milestones: 0.8.1, 0.8.2

Jul 18, 2018

murrayrm mentioned this pull request

Jul 27, 2018

Fix pzmap not showing zeroes #230

Closed

Sup3rGeo commented Oct 2, 2018

Copy link

Copy Markdown

Author

gentle ping

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

murrayrm approved these changes

Dec 28, 2018

View reviewed changes

murrayrm merged commit 32b0c2b into python-control:master

Dec 28, 2018

murrayrm mentioned this pull request

Apr 11, 2019

ComplexWarning while trying to plot a pzmap #288

Open

bnavigator mentioned this pull request

May 3, 2021

Cover more of root_locus: dtime and sisotool #617

Merged

murrayrm mentioned this pull request

Jun 16, 2021

Two 'sgrid' functions when we only need one? #634

Open

Reviewers

murrayrm

murrayrm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.8.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Sup3rGeo commented Feb 19, 2018

Uh oh!

coveralls commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murrayrm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sup3rGeo commented Mar 11, 2018

Uh oh!

abouatef commented Apr 25, 2018

Uh oh!

jwdinius commented May 13, 2018

Uh oh!

Sup3rGeo commented May 14, 2018

Uh oh!

murrayrm commented May 14, 2018

Uh oh!

Sup3rGeo commented Oct 2, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 19, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.