Move matlab compatibility routines to subpackage #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Define __all__ variables in the modules so that
from .<module> import *
imports only the functions/classes we use. This is probably a little
better than listing them explicitly in __init__.py, since then if we add
a new function to a module, we only need to change the __all__ variable
in the same module, rather than edit __init__.py. Furthermore it avoids
duplicating some of these listings in matlab/__init__.py.
The dcgain function is now implemented as a method of the StateSpace and TransferFunction classes. There is also a routine lti.dcgain(sys) that simply calls sys.dcgain(). Note that the DC gain is now returned as a numpy array (not a matrix), and furthermore, single dimensions are removed with np.squeeze. This is a change from the previous implementation in matlab.dcgain(). The new convention makes it easier to deal with SISO systems (a common case), since instead of referring to a gain as g[0][0], you can refer to it simply as g. This seems preferable to me than returning a np.matrix. (The new convention is also more consistent with Matlab.)
Matlab module documentation is now in the same format as the main module, with one routine per html page.
Previously, bode_plot returned phase and magnitude in different units (dB/absolute magnitude, radians/degrees) depending on the values of boolean flags. This was inconsistent with the documentation and the tests. This is now made consistent, so that bode_plot always returns absolute magnitude and phase in radians, and the documentation is consistent with the behavior.
2 similar comments
murrayrm
added a commit
that referenced
this pull request
May 17, 2015
Move matlab compatibility routines to subpackage. Reviewed all changes and confirmed that these should give the same functionality, modulo the changes @cwrowley noted in the PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request removes dependencies of the main
controlpackage on thematlabcompatibility module, and makes the Matlab compatibility routines a separate subpackage. One advantage of this reorganization is that different default options can be used (e.g., for Bode plots) when the Matlab compatibility module is used.One bug that surfaced when making this organization was an inconsistent handling of optional arguments specifying units in
control.bode_plot. Previously, if one specifieddB=True, then the values were returned in dB instead of absolute magnitude, which was inconsistent with the documentation and the tests. (The intended, documented behavior withdB=Trueis that the magnitude is plotted in dB, but returned as an absolute magnitude.)The documented behavior of the phase was different: the phase was returned in degrees if
deg=Truewas specified, otherwise radians. This PR changes this behavior to always return phase in radians, so that the optional argument determines only how phase is plotted, not the return values. This is consistent with how magnitude is handled, and is probably better, since the behavior is less "surprising" when different default options might be specified (e.g.,dB=Trueforcontrol.matlabordB=Falseotherwise).