Reimplement Kaiser window

[alvgaona]

Relates to #85366

• For reference follow Implement old windows in Python #87082.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/87330

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 38550c5:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[alvgaona]

@mruberry @lezcano I'm getting this lint error.

>>> Lint for torch/__init__.py:

  Error (MYPY) [no-redef]
    Name "kaiser_window" already defined (possibly by an import)

        874  |
        875  |# Importing alias functions due to backward compatibility.
        876  |# Link to a detailed conversation: https://github.com/pytorch/pytorch/pull/87082
    >>> 877  |from torch._windows import kaiser_window
        878  |
        879  |# Quantized, sparse, AO, etc. should be last to get imported, as nothing
        880  |# is expected to depend on them.

I feel like we need to somehow stop generating the Python bindings for the C++ implementation for this warning to get solved. Additionally, I also feel like we should remove this block as it has old documentation and doesn't really add anything. We're doing the alias to a different implementation; at the moment it gets overridden by the alias function's docstring.

pytorch/torch/_torch_docs.py

Lines 12723 to 12765 in c9b6184

	add_docstr(
	torch.kaiser_window,
	"""
	kaiser_window(window_length, periodic=True, beta=12.0, *, dtype=None, \
	layout=torch.strided, device=None, requires_grad=False) -> Tensor
	"""
	+ r"""
	Computes the Kaiser window with window length :attr:`window_length` and shape parameter :attr:`beta`.
	Let I_0 be the zeroth order modified Bessel function of the first kind (see :func:`torch.i0`) and
	``N = L - 1`` if :attr:`periodic` is False and ``L`` if :attr:`periodic` is True,
	where ``L`` is the :attr:`window_length`. This function computes:
	.. math::
	out_i = I_0 \left( \beta \sqrt{1 - \left( {\frac{i - N/2}{N/2}} \right) ^2 } \right) / I_0( \beta )
	Calling ``torch.kaiser_window(L, B, periodic=True)`` is equivalent to calling
	``torch.kaiser_window(L + 1, B, periodic=False)[:-1])``.
	The :attr:`periodic` argument is intended as a helpful shorthand
	to produce a periodic window as input to functions like :func:`torch.stft`.
	.. note::
	If :attr:`window_length` is one, then the returned window is a single element tensor containing a one.
	"""
	+ r"""
	Args:
	window_length (int): length of the window.
	periodic (bool, optional): If True, returns a periodic window suitable for use in spectral analysis.
	If False, returns a symmetric window suitable for use in filter design.
	beta (float, optional): shape parameter for the window.
	Keyword args:
	{dtype}
	layout (:class:`torch.layout`, optional): the desired layout of returned window tensor. Only
	``torch.strided`` (dense layout) is supported.
	{device}
	{requires_grad}
	""".format(
	**factory_common_args
	),
	)

One last question. What should I do next? I got lost a little bit in the previous conversation about what to do with the C++ implementations, the PrimTorch, and the _refs directory (still learning about this kind of module).

[lezcano]

You can stop PyTorch from generating bindings by adding a manual_cpp_binding: True to its corresponding entry in native_functions.yaml.
About the docs, agree. You should replace those docs with docs of other functions that are alias, as Mike suggested in the previous PR.
About the refs, it may be fine not to do anything on that end really (unless @mruberry thinks otherwise). Already having this implemented in Python is a massive step forward, and adding these to the _refs folder (which, as you may have already seen, is a folder with Python implementations of many PyTorch functions) should be fairly straightforward and can be done at a later time.

[mruberry]

This looks great; I would just leave torch.kaiser_window as-is for the moment and focus on the development of torch.signal.windows. (That thought goes for the development of additional windows, too). I made some small inline comments about torch.signal.windows.kaiser for your review.

We can review aligning the existing torch windowing operations with torch.signal.windows later.

[alvgaona]

@mruberry addressed a couple of your comments with additional comments 😛

[lezcano]

I did mention we may want to update the C++ implementations, but I think it's alright to leave them as they are for now, so that we don't generate unnecessary extra work.

[alvgaona]

You can stop PyTorch from generating bindings by adding a manual_cpp_binding: True to its corresponding entry in native_functions.yaml. About the docs, agree. You should replace those docs with docs of other functions that are alias, as Mike suggested in the previous PR. About the refs, it may be fine not to do anything on that end really (unless @mruberry thinks otherwise). Already having this implemented in Python is a massive step forward, and adding these to the _refs folder (which, as you may have already seen, is a folder with Python implementations of many PyTorch functions) should be fairly straightforward and can be done at a later time.

@lezcano I tried using manual_cpp_binding: True but it fails. I guess we can have a look at the follow-up PR where we try to do the aliasing work.

[lezcano]

follow up PR LGTM!

[alvgaona]

@lezcano @mruberry I think we can merge it and go back to #87082, right?

[ngimel]

check for beta should go before returning the wrong value for M=1

[mruberry]

What's interesting is that SciPy does not require a positive beta:

scipy.signal.windows.kaiser(1, beta=-5)
: array([1.])

scipy.signal.windows.kaiser(10, beta=-5.)
: array([0.03671089, 0.20127873, 0.47552746, 0.7753221 , 0.97273069,
       0.97273069, 0.7753221 , 0.47552746, 0.20127873, 0.03671089])

The wikipedia entry stipulates that beta should be non-negative, but not strictly positive: https://en.wikipedia.org/wiki/Kaiser_window

So, follow-up questions:

• why are we requiring a positive beta?
• should the documentation discuss this requirement?

[ngimel]

Sure, let's make it non-negative. Definition includes I0(beta) and I0 is not defined for negative args (and there's no alternative definition for 1), so looks like scipy is wrong.

[alvgaona]

Agreed with @ngimel. I'll turn that precheck into a non-negative one, and I'll move it up as well.

[alvgaona]

@lezcano @mruberry @ngimel I guess for the gaussian and exponential windows we should move the std and tau check up a few lines too (will do in another PR).

[lezcano]

yup. Feel free to add it to this PR if oyu want.

[alvgaona]

I'll do it in #87082

[mruberry]

Agreed with @ngimel. I'll turn that precheck into a non-negative one, and I'll move it up as well.

But why not just allow negative betas, too?

[alvgaona]

@mruberry in the literature beta is always non-negative.

[mruberry]

I understand, just interesting that SciPy allows them

[lezcano]

Leaving aside a small point I left and Natalia's valid point, this LGTM. Let's see what Mike has to say though.

As always, thank you for doing all this ground work!

[mruberry]

See one suggestion about mentioning non-negative requirement for beta in the documentation, otherwise LGTM! Thanks @alvgaona

[alvgaona]

@mruberry @lezcano ready to get merged. Is it possible to do a squash merge?

[lezcano]

@pytorchbot merge

[pytorchmergebot]

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

[github-actions]

Hey @alvgaona.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.