Fixed allclose call in normalize() of filter_design.py and added a test ... #259

Merged
merged 1 commit into from Sep 8, 2012

Projects

None yet

5 participants

@wa03

Fixed signal.normlize so that the allclose() calls won't give false positives on the filter coefficients.
Unsure why b is rank-2, but for rank-1 input b arrays, nothing should be changed.

Added a unit test to compare against the same filter in MATLAB to make sure the output coefficients are the same.

Further details and explanation on Gmane:
http://thread.gmane.org/gmane.comp.python.scientific.devel/16542/

@wa03 wa03 Fixed allclose call in normalize() of filter_design.py and added a te…
…st so check the output against the same filter coefficients from MATLAB
4895a52
@rgommers
SciPy member

@WarrenWeckesser @teoliphant @cournape : anyone have time to look at this?

Josh asked on the list to have this included in 0.11.0, which is still possible.

@sturlamolden

This is a bug, the relative tolerance becomes zero: 0 * rtol = 0

@sturlamolden

This should preferably use a relative tolerance

allclose(0, outb[:, 0], rtol=1e-14)

ditto for the rest

Changing the order of argument a and b to allclose is correct.

@sturlamolden

Added line comments.

I believe rtol should be kept, but the order of arguments to allclose must be swapped.

@wa03

Thanks. I'm unclear on this part. Should I rebase with the changes so there's only 1 commit to master or should I just make the change and commit it?

EDIT: Nevermind, I misunderstood pull requests on github.

@jakevdp
SciPy member

Hi,
I tested this, and I think it's good to go as-is.

Regarging the atol vs rtol issue: I think it makes sense to use absolute tolerance for comparing a value to zero. If we use rtol=1E-14 and leave the default atol=1E-8, then it is atol which drives the result.

@charris
SciPy member

You can set atol to less. It is standard practice to use both, usually internally as atol + abs(val)*rtol. val doesn't need to be very large before the relative tolerance dominates. If the routine doesn't use both in that way it is a bug.

@jakevdp
SciPy member

@charris: yes, but my point is that when you call

np.allclose(0, a)

as above, then it makes little sense to worry about what rtol is (responding to @sturlamolden's suggestion), because abs(0 - a) will always be much larger than rtol * abs(a). The choice of atol is what is important when comparing a value to zero. That is why I think this PR is fine to be merged as it stands.

@charris charris closed this Jul 17, 2012
@charris charris reopened this Jul 17, 2012
@rgommers
SciPy member

If the values in a don't get larger than O(1), then @jakevdp is right. Which is the case here. So I also don't see a problem here.

@rgommers
SciPy member

Either way works, so I'm going to already include the patch as is in my 0.11.x branch in order to build RC1 tonight. Can always be changed later, and shouldn't affect how this ends up in master in the end.

@wa03

Okay sounds good.

@rgommers
SciPy member

For the record, this is now in 0.11.x as 8bd46f4

@rgommers
SciPy member

I'd like to close this PR. @charris, if you still think it's better to include atol then I'll fix this before merging, just let me know.

@rgommers
SciPy member

No response on the final minor point (atol), so I'll merge this now as is.

@rgommers rgommers merged commit 14b1e07 into scipy:master Sep 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment