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. #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. Having such tests in place is useful for Scipy for several reasons --- for instance, it makes easier to check that new contributions work, and having them in place makes it more certain that any future changes in code (for instance in other parts of Scipy) do not break the functionality. Normally, I or someone else of the regulars can of course write these up themselves, but as Scipy spans a wide field of topics, it can take a while until someone with the correct expertise has time for it... So, it's really helpful if pull requests come with tests. Thanks, -- Pauli Virtanen

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

On 3 June 2012 01:12, Pauli Virtanen <pav@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