Implement torch.kaiser_window #44271

muthuArivoli · 2020-09-07T05:29:56Z

Related to #38349

dr-ci · 2020-09-07T06:24:15Z

💊 CI failures summary and remediations

As of commit a5455f8 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 47 times.

muthuArivoli · 2020-09-07T22:02:58Z

@mruberry PTAL. The XLA failure is real, but I'm not sure why it works for the other window functions. but fails on kaiser_window (maybe because I'm using a kernel for kaiser).

In response to the comments from the i0 PR:

I believe I have fused the operations (not completely sure if I did it correctly, though). I placed the kernel in the UnaryOpsKernel files, since there was no file in the cpu directory for TensorFactories, but I can create one if that is needed.
This function currently uses the periodic arg, which acts opposite to the sym arg in scipy.signal. I did this because all of the other pytorch window functions use this periodic arg, and do not use the sym version.

aten/src/ATen/native/cpu/UnaryOpsKernel.cpp

aten/src/ATen/native/TensorFactories.cpp

aten/src/ATen/native/cpu/UnaryOpsKernel.cpp

aten/src/ATen/native/cuda/UnaryOpsKernel.cu

mruberry · 2020-09-08T14:22:42Z

test/test_torch.py

+                    res = torch_method(size, *args, periodic=periodic, device=device)
                    # NB: scipy always returns a float32 result
-                    ref = torch.from_numpy(signal.get_window(name, size, fftbins=periodic))
+                    ref = torch.from_numpy(signal.get_window((name, *args), size, fftbins=periodic))


I don't think the signature for get_window() will do what you like. Instead you can pass the torch function and the reference as a pair:

(torch.kaiser_window, scipy.signal.kaiser)

You could also pass the args to each in the tuples so you can unpack them as:

torch_fn, torch_args, ref_fn, ref_ags = ...

test/test_torch.py

mruberry · 2020-09-08T14:27:19Z

test/test_torch.py

            test(window)

+        for num_test in range(50):
+            test('kaiser', (random.random() * 30,))


It'd be nice to see a bigger cross of window sizes and beta params, that can be crossed with the periodic argument, too.

mruberry · 2020-09-08T14:28:10Z

torch/_torch_docs.py

+kaiser_window(window_length, beta, periodic=True, dtype=None, \
+layout=torch.strided, device=None, requires_grad=False) -> Tensor
+""" + r"""
+Kaiser window function.


Would a more accurate description be something like, "Computes symmetric samples from the Kaiser windowing function"?

I don't think I agree with this, since you can compute non symmetric samples with this function (in fact the default is to do that).

That makes sense, I realize now what I suggested is a problem. What I'm trying to get at is something more like, "samples symmetrically from..." as in this Wikipedia article. But all the other documentation, like NumPy and matlab, seem to just refer to this as computing the Kaiser window function, which doesn't seem correct but is probably good enough for now.

What about just, "Computes the Kaiser window with window length :attr:`window_length` and shape parameter :attr:`beta`."? It's nice to include how the positional arguments relate to the function in the opening sentence. I don't love the user of the term "shape factor" for beta, but it seems reasonable and consistent with other descriptions.

torch/_torch_docs.py

…window

muthuArivoli · 2020-09-14T22:47:38Z

@mruberry, Unfortunately, the doc build keeps failing if I list out the keyword args, giving me the same error as in #43669. I also tried the fix recommended in that issue, and it still failed to build. I have removed the keyword args description, but kept the star in the function description. What is the right way to proceed?

mruberry · 2020-09-14T23:09:04Z

@mruberry, Unfortunately, the doc build keeps failing if I list out the keyword args, giving me the same error as in #43669. I also tried the fix recommended in that issue, and it still failed to build. I have removed the keyword args description, but kept the star in the function description. What is the right way to proceed?

That's the best we can do for now. Sorry about the docs build. We should really root cause and fix this issue.

muthuArivoli · 2020-09-15T00:24:29Z

@mruberry, Unfortunately, the doc build keeps failing if I list out the keyword args, giving me the same error as in #43669. I also tried the fix recommended in that issue, and it still failed to build. I have removed the keyword args description, but kept the star in the function description. What is the right way to proceed?

That's the best we can do for now. Sorry about the docs build. We should really root cause and fix this issue.

Alright, in that case I think this can use another review, hopefully the content in the docs is better now.

mruberry · 2020-09-15T09:18:37Z

aten/src/ATen/native/native_functions.yaml

+- func: kaiser_window(int window_length, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
+  use_c10_dispatcher: full
+
+- func: kaiser_window.periodic(int window_length, bool periodic, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor


Can these three native_functions.yaml entries be combined if defaults for periodic and beta are specified?

- func: kaiser_window.beta(int window_length, bool periodic=True, float beta=12, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor use_c10_dispatcher: full

torch/_torch_docs.py

aten/src/ATen/native/TensorFactories.cpp

mruberry

Great work, @muthuArivoli! i0 and this are really interesting functions and it's great to have them available in PyTorch.

I have a couple nits about the docs. I also think you could get away with a single dispatch for this function, too, but it's OK if you'd prefer to leave the three dispatch entries you have (that's consistent with the other window functions, after all). Let me know when you're satisfied and we'll get this merged!

…window

muthuArivoli · 2020-09-15T20:04:29Z

@mruberry, I updated the docs. I tried implementing the single dispatch, but I ran into this compile error:

../torch/csrc/autograd/generated/variable_factories.h:371:72: error: ‘True’ was not declared in this scope
 inline at::Tensor kaiser_window(int64_t window_length, bool periodic = True, double beta = 12.0, const at::TensorOptions & options = {}) {
                                                                        ^~~~

I tried to make the True lowercase in the yaml file, but this still popped up. If this is due to something not related to the implementation of the kaiser window function, then I think I'll stick with the multiple dispatches, and this should be good to merge.

mruberry · 2020-09-15T20:14:03Z

@mruberry, I updated the docs. I tried implementing the single dispatch, but I ran into this compile error:
../torch/csrc/autograd/generated/variable_factories.h:371:72: error: ‘True’ was not declared in this scope
 inline at::Tensor kaiser_window(int64_t window_length, bool periodic = True, double beta = 12.0, const at::TensorOptions & options = {}) {
                                                                        ^~~~
I tried to make the True lowercase in the yaml file, but this still popped up. If this is due to something not related to the implementation of the kaiser window function, then I think I'll stick with the multiple dispatches, and this should be good to merge.

That's weird. See, for example:

pytorch/aten/src/ATen/native/native_functions.yaml

Line 97 in 69839ea

- func: requires_grad_(Tensor(a!) self, bool requires_grad=True) -> Tensor(a!)

which sets a bool to a default value. We don't need to worry about it in this PR, however. Let's wait for the tests and get it merged.

codecov · 2020-09-16T02:04:32Z

Codecov Report

Merging #44271 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #44271      +/-   ##
==========================================
- Coverage   67.95%   67.95%   -0.01%     
==========================================
  Files         384      384              
  Lines       49768    49769       +1     
==========================================
- Hits        33822    33821       -1     
- Misses      15946    15948       +2

Impacted Files	Coverage Δ
torch/overrides.py	`98.05% <ø> (ø)`
torch/_torch_docs.py	`100.00% <100.00%> (ø)`
torch/utils/_benchmark/utils/common.py	`77.68% <0.00%> (-1.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d733d6...a5455f8. Read the comment docs.

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-09-17T06:18:57Z

@mruberry merged this pull request in b61d3d8.

mruberry · 2020-09-17T06:28:11Z

This is merged! Congrats @muthuArivoli. Let me know if you're looking for something else and, if you are, what might be fun.

muthuArivoli · 2020-09-17T06:58:13Z

Yes, I'm looking for something else. Any recommendations?

mruberry · 2020-09-17T07:13:25Z

It depends on what you're looking for. Let me offer a few options:

torch.divmod would be nice. I think contributors have avoided it because it returns two tensors instead of one.
We're starting to invest more in linear algebra operations, like improving our Cholesky decomposition. These are especially challenging tasks but one of them might be interesting.
kron would be a tricky tensor factory to implement.
pad would be useful. PyTorch already has a pad function but there are discrepancies between our Python and C++ APIs, and NumPy supports many more modes (making pad consistent and/or support even one new mode would be nice).

Would any of those be interesting? Or would you like something simpler as a break? I can break down linear algebra-related ideas further if that direction is interesting.

muthuArivoli · 2020-09-17T07:24:00Z

I'd like to hear more about the linear algebra ideas.

One other question. When implementing the specialized splits (hsplit, dsplit, vsplit), I ran into a problem where the arg name for the input didn't match up with the arg name in the torch.split function. For some reason the torch.split uses the arg name tensor as its input, but I could only get the arg name input to work. Any ideas on how to fix that/is that important?

mruberry · 2020-09-17T07:29:05Z

I'd like to hear more about the linear algebra ideas.

I'll pull a few and update the linalg rollup issue soon. It's a really interesting space.

One other question. When implementing the specialized splits (hsplit, dsplit, vsplit), I ran into a problem where the arg name for the input didn't match up with the arg name in the torch.split function. For some reason the torch.split uses the arg name tensor as its input, but I could only get the arg name input to work. Any ideas on how to fix that/is that important?

That is strange. I thought we required native_functions always start with self, but I might be mistaken. Do you have a draft PR I could look at? Also, it seems like the first arg in torch.split should be called input in the documentation, to be consistent with other functions.

Summary: Related to #38349 Pull Request resolved: #44271 Reviewed By: ngimel Differential Revision: D23727972 Pulled By: mruberry fbshipit-source-id: b4c931b2eb3a536231ad6d6c3cb66e52a13286ac

muthuArivoli added 2 commits September 6, 2020 22:48

initial implementation

ad96f7d

add kernels

574e620

muthuArivoli marked this pull request as draft September 7, 2020 05:30

pytorchbot added the open source label Sep 7, 2020

fix cuda

c7b6405

zhangguanheng66 requested a review from mruberry September 7, 2020 14:38

zhangguanheng66 added module: numpy Related to numpy support, and also numpy compatibility of our operators module: operators triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 7, 2020

muthuArivoli added 2 commits September 7, 2020 11:55

add headers

b31c73a

fix docs and attempt to fix xla

e34d388

muthuArivoli marked this pull request as ready for review September 7, 2020 21:48