[SciPy-Dev] New contribution: bode() function for LTI systems

[Ralf Gommers]

On Sun, Jun 3, 2012 at 7:07 PM, Bjørn Forsman <bjorn.forsman at gmail.com>wrote:

> On 3 June 2012 01:12, Pauli Virtanen <pav at iki.fi> wrote:
> > 02.06.2012 20:16, Bjørn Forsman kirjoitti:
> > [clip]
> >> https://github.com/scipy/scipy/pull/224 - ENH: ltisys: new bode()
> function
> >> https://github.com/scipy/scipy/pull/225 - ENH: ltisys: make lti zpk
> >> initialization work with plain lists
> >
> > #225 is obvious, and should go in.
>
> I see it has been merged. Thanks.
>
> > #224 -- looks mostly good. However, you don't include a test case, and
> > as I'm not familiar with the problem domain, I cannot easily
> > double-check that the code works (and therefore, cannot merge it).
> >
> > Could you include a (simple but nontrivial) calculation where you know
> > the correct result, and that uses this code?  You most likely have
> > already done a couple of checks like this already, so coding up one or a
> > few of them as test cases hopefully is not too much work.
>
> I have checked this bode() against the bode function in Matlab.
>
> I have now added (and pushed) 4 tests; test 1 and 2 sanity-checks the
> bode() magnitude and phase data against manually entered data. Test 3
> and 4 calculate the magnitude and phase using the same algorithm as
> the bode() function, but without actually *using* bode(). This way
> you/we will be able to catch regression bugs.
>
> Let me know if there is anything more I need to do to get this merged.
>

Those tests look good.

>
> BTW, I think #224 will cause a small merge conflict in test_ltisys.py.
> How do you prefer to handle this? I normally rebase but some people
> don't like rebasing (so I thought I'd ask before doing it) and I don't
> know what github does to a pull-request if I rebase the branch.
>

You can rebase and push again to the same branch (you need to force push).
The only reason not to do that would be if there are a lot of comments
directly on commits, because they'll be lost after rebasing. In this case
there aren't, so rebasing is preferred to opening a new PR.

Ralf
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scipy-dev/attachments/20120603/702244e1/attachment.html>