Skip to content

Conversation

@ycastorium
Copy link
Contributor

Similar solutions usually go with the Chebyshev polynomials, i decided to use the Polynomial/Asymptotic approach instead, the difference should not be that perceptible.

I can change do Chebyshev though.

Copy link
Collaborator

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

This is looking good! I left a few small comments.

Comment on lines 368 to 372
deftransformp pop_window_size(opts) do
{n, opts} = Keyword.pop(opts, :n)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should refactor all functions here to be deftransforms where n is the first argument instead. Let's do this in a separate PR though.

Nx.exp(abs_x) / Nx.sqrt(2 * Nx.Constants.pi() * abs_x) *
(1.0 + 1.0 / (8.0 * abs_x) + 9.0 / (128.0 * Nx.pow(abs_x, 2)))

Nx.select(abs_x < 3.75, small_x_result, large_x_result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you use if abs_x < 3.75 do ... else ... end, Nx will compile it to a cond block which is a bit more efficient in that it'll run one branch only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the if would work in this case, abs_x is not a scalar.

end
end

defnp i0(x) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

should probably rename this function to something more meaningful, or at least kaiser_i0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ycastorium ycastorium force-pushed the adds_kaiser_window branch 2 times, most recently from 7cb46b4 to 32fa8e3 Compare March 3, 2025 12:22
@ycastorium ycastorium force-pushed the adds_kaiser_window branch from 62368c4 to 183f8d3 Compare March 3, 2025 12:24
@ycastorium ycastorium requested a review from polvalente March 3, 2025 13:59
@polvalente
Copy link
Collaborator

Thanks! Merging this and I'll apply the refactor I suggested for extracting n into a mandatory argument

@polvalente polvalente merged commit e42d747 into elixir-nx:main Mar 5, 2025
2 checks passed
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.

2 participants