Discrete omega-damping plot and tweaks by Sup3rGeo · Pull Request #193 · python-control/python-control

Discrete omega-damping plot and tweaks by Sup3rGeo · Pull Request #193 · python-control/python-control

Other tweaks:

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

Victor added 4 commits

February 19, 2018 11:06

(cherry picked from commit bfd9279)

(cherry picked from commit 382b4e9)

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

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.


		__all__ = ['pzmap']

		def zgrid(plt):

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.

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?

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.

		__all__ = ['pzmap']

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

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.

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.


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

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

Choose a reason for hiding this comment

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

ok

		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)

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.

		if (Plot):
		import matplotlib.pyplot as plt

		if isdtime(sys):

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).

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)?

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).

		import matplotlib.pyplot as plt

		if isdtime(sys):
		zgrid(plt)

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.

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.

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 -:).

…zgrid to new python module.

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.

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

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?

@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

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.