GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Improve markov function, add mimo support, change api to TimeResponseData by KybernetikJo · Pull Request #1022 · python-control/python-control Conversation
|
|
Rather than breaking old code, it would be nice to do this in a way that preserved prior functionality. So use It also seems a bit odd (to me) to have the return type be a |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I will update this one.
I do not have strong feelings about that. The idea was to simplify the joint use of markov and era: impulse_response = ct.markov(response, m=10) sysd = ct.era(impulse_response, r=4) Direct access of Markov parameters: _, H = ct.markov(response, m=10) I can change the return value to an array. I think it is also better for the old API. H = ct.markov(Y, U, m=10) sysd = ct.era(H, r=4) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
cee59d1 to
a3276b7
Compare
There was an error while loading. Please reload this page.
|
coverage: 94.669% (+0.04%) from 94.629% |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks OK. It would be good to add unit tests to cover some of the lines that are not currently covered.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| """markov(Y, U, [, m]) | ||
|
|
||
| Calculate the first `m` Markov parameters [D CB CAB ...] | ||
| from data |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Should end in a period.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if Umat.shape[0] != 1 or Ymat.shape[0] != 1: | ||
| raise ControlMIMONotImplemented | ||
| # Get the system description | ||
| if (len(args) < 1): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Parenthesis are not needed here. The more standard style convention would be
if len(args) < 1:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| elif (len(args) > 2): | ||
| raise ControlArgument("too many positional arguments") | ||
| else: | ||
| if (len(args) < 2): |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
OK to remove parens (here and below).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| dt : True of float, optional | ||
| True indicates discrete time with unspecified sampling time, | ||
| positive number is discrete time with specified sampling time.It | ||
| can be used to scale the markov parameters in order to match the |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Markov should be capitalized.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| dt : True of float, optional | ||
| True indicates discrete time with unspecified sampling time, | ||
| positive number is discrete time with specified sampling time.It | ||
| can be used to scale the markov parameters in order to match the |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
In the case that a TimeResponseData object is given, should dt default to the value coming from the time array? You could set the default value of dt to None and then if the input is Y, U then you default to dt=1 and the input is data, you default to data.time[1] - data.time[0] (perhaps with a check to make sure that time points are equally spaced?).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
fd87d92 to
e61ac53
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.