Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filtering (filt) across columns of arrays and method signature tweaks #7560

Merged
merged 3 commits into from
Jul 11, 2014

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jul 10, 2014

Here are yet two more patches for filt and filt! following #7513.

1. Enable filtering (filt) across columns of arrays.

When passed a multidimensional array, simply iterate over its columns, independently filtering each one. The starting filter state may be given as a vector (in which case it is common across all columns) or an array with the same number of columns as the input. This is a pretty common feature and is very handy.

cf. JuliaDSP/DSP.jl#58 (comment)

2. Relax filt signatures to allow heterogeneous types

Previously, filt[!] methods were only defined for arguments of all the same type T<:Number. This relaxes that restriction, allowing one to, for example, filter an array of Float32s by Float64 coefficients. Instead of defining many type parameters (T<:Number, S<:Number, R<:…), filt now simply trusts that the eltype of the arrays have methods for basic arithmetic. By default, the output of filt will be determined by promote_type of all arguments.

I needed this second change to simplify filtering of arrays of Float32s. It should generally be more forgiving.

cc: @simonster, @timholy, @codles

When passed a multidimensional array, simply iterate over its columns, independently filtering each one.  The starting filter state may be given as a vector (in which case it is common across all columns) or an array with the same number of columns as the input.
Previously, filt[!] methods were only defined for arguments of all the same type T<:Number. This relaxes that restriction, allowing one to, for example, filter an array of Float32s by Float64 coefficients.

By default, the output of filt will be the determined by promote_type of all arguments.

Instead of defining many type parameters (T<:Number, S<:Number, ...), filt now simply trusts that the eltype of the arrays have methods for basic arithmetic.
function filt!{T<:Number}(b::Union(AbstractVector{T}, T), a::Union(AbstractVector{T}, T), x::AbstractVector{T};
si::AbstractVector{T}=zeros(T, max(length(a), length(b))-1), out::AbstractVector{T}=x)
function filt!{T}(b::Union(AbstractVector, Number), a::Union(AbstractVector, Number), x::AbstractArray{T};
si::AbstractArray=_zerosi(b,a,T), out::AbstractArray=x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have the type inference issue I mentioned here if called with keyword arguments, since out is a keyword argument but determines the return type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh. I think you're right. That's frustrating. The same will be true for si in filt above since it's a part of the type promotion. That one is easier to make a positional argument, but filt! would ideally have two optional arguments, and with one in front it'd be ambiguous with positional args.

Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short of fixing type inference, I like the general pattern of having output arguments be first (they're closer to the left-hand side, which is where outputs usually go). You could make out a required argument for filt!, forcing the user to say filt!(x, b, a, x) when you want to filter in-place.

Because we're back to only one optional element for both `filt` and `filt!` (the initial state), I figure it's simpler to just use a positional argument in both cases.  So now the API is:

    filt(b, a, x, [si])
    filt!(out, b, a, x, [si])
@mbauman
Copy link
Member Author

mbauman commented Jul 11, 2014

Alright, yet another API change to fix the type instability. Sorry @codles!

Because we're back to only one optional element for both filt and filt! (the initial state), I figure it's simpler to just use a positional argument in both cases. So now the API is:

filt(b, a, x, [si])
filt!(out, b, a, x, [si])

This will still leave room in the future for a 3-argument version of filt! with keywords if the inference improves. I think it should be a good stable place to start. (Just for reference, the only other argument SciPy supports is an axis, but I think Julia would be better off not supporting such a thing.)

@timholy
Copy link
Member

timholy commented Jul 11, 2014

Actually, I was already wondering if this should (someday) be generalized to perform filtering across any dimension, which is what I presume axis means. In julia there are many examples of such functions (see sum, maximum, fft, etc). It's usually called dim, dims, or region; I actually think axis/axes might be the clearer terminology. But I don't need it now, and if you don't either then might as well wait until there's call to implement it. axis wouldn't affect inference at all, so that could be a keyword if/when someone needs this.

@timholy
Copy link
Member

timholy commented Jul 11, 2014

I'm good with this. @simonster, if you like it, merge.

@mbauman
Copy link
Member Author

mbauman commented Jul 11, 2014

Hah, fair enough. I probably shouldn't make such grand, sweeping statements. I don't need this, and I don't want to be responsible for maintaining the additional complexity. :)

Thanks again, gentlemen. I know it takes time for you to review these things, but your feedback is invaluable as I continue learning.

simonster added a commit that referenced this pull request Jul 11, 2014
Filtering (filt) across columns of arrays and method signature tweaks
@simonster simonster merged commit fdfccd7 into JuliaLang:master Jul 11, 2014
@mbauman mbauman deleted the filtarray branch July 11, 2014 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants