[SciPy-Dev] New contribution: bode() function for LTI systems
Sun Jun 3 12:57:55 CDT 2012
On Sun, Jun 3, 2012 at 7:07 PM, Bjørn Forsman <firstname.lastname@example.org>wrote:
> On 3 June 2012 01:12, Pauli Virtanen <email@example.com> wrote:
> > 02.06.2012 20:16, Bjørn Forsman kirjoitti:
> > [clip]
> >> https://github.com/scipy/scipy/pull/224 - ENH: ltisys: new bode()
> >> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the SciPy-Dev