Add istft operation #2029

alealv · 2023-10-24T19:12:04Z

Close #2016

This code adds ISTFT op

It was a bit challenging to go through it because I needed to learn available mil operations and adapt the process.
I based my self on this PR as suggested by junepiz

There are still many things I'm not confident with and I would like some help if possible:

Understanding notation used in MLOps
Proper testing of the function
Why does STFT doesn't use center. The same would apply to ISTFT
Check if the for loop in the OLA (overlap-add) function is fine or I need to use some special computation

coremltools/converters/mil/frontend/torch/test/test_torch_ops.py

coremltools/converters/mil/mil/ops/defs/complex_dialect_ops.py

junpeiz · 2023-11-15T08:48:46Z

coremltools/converters/mil/frontend/torch/test/test_torch_ops.py

+            [None, 60], # length
+        )
+    )
+    def test_istft(self, compute_unit, backend, input_shape, complex, n_fft, hop_length, win_length, window, center, pad_mode, normalized, onesided):


This test is great with comprehensive coverages! Besides this, as istft is the inverse of stft, I'm wondering if it's possible to add another test case, where the torch model first call torch.stft, and then feed the output back to torch.istft to restore the original input?

coremltools/converters/mil/mil/ops/defs/complex_dialect_ops.py

coremltools/converters/mil/mil/passes/defs/lower_complex_dialect_ops.py

junpeiz

Thank you for your contributions! Left some comments.

Maybe it's because that this PR is still in draft stage (I noticed the Draft in PR title), but just want to mention that there are some undefined vars which triggers flake8 checking error: https://gitlab.com/coremltools1/coremltools/-/jobs/5539997442.

alealv · 2023-11-15T15:48:14Z

Thank you for your contributions! Left some comments.

Maybe it's because that this PR is still in draft stage (I noticed the Draft in PR title), but just want to mention that there are some undefined vars which triggers flake8 checking error: https://gitlab.com/coremltools1/coremltools/-/jobs/5539997442.

This should be fixed now.

alealv · 2023-11-15T15:50:54Z

I still don't understand how you handle the center parameter in the STFT implementation

We should also handle it in the ISTFT in a similar manner.

junpeiz · 2023-11-23T12:48:32Z

I still don't understand how you handle the center parameter in the STFT implementation

We should also handle it in the ISTFT in a similar manner.

@alealv That's a really good question.

As shown in STFT PyTorch doc, the center is only for padding the input. The center parameter is handled by PyTorch ops lowering, which means coremltools does not even see this center parameter when it looks at the Torch IR graph.

More specifically, when we lower the STFT op in STFT lowering function, the input_data already reflects the center parameter, so the center parameter is not used in mb.complex_stft. To better understand it, feel free to play with the test_stft test case in coremltools/converters/mil/frontend/torch/test/test_torch_ops.py: When input_shape is (1, 32) and center=False, the input_data in STFT lowering function is still (1, 32). However, when center is True, the input_data in STFT lowering function will become (1, 48).

alealv · 2024-01-02T13:00:31Z

I finally fixed all issues and tests pass. The only result I found weird is here

        elif length and return_complex:
            with pytest.raises(ValueError, match="New var type `<class 'coremltools.converters.mil.mil.types.type_tensor.tensor.<locals>.tensor'>` not a subtype of existing var type `<class 'coremltools.converters.mil.mil.types.type_tensor.tensor.<locals>.tensor'>`"):
                TorchBaseTest.run_compare_torch(
                    input_shape,
                    ISTFTModel(),
                    backend=backend,
                    compute_unit=compute_unit
                )

TobyRoseman · 2024-01-02T18:18:58Z

@alealv - Your branch is a couple months old at this point. Please rebase your changes on the current tip of main. You probably want to squash all your changes first. Once that is done, we can kick off a CI run.

@junpeiz - please re-review these changes.

alealv · 2024-01-02T19:23:21Z

@alealv - Your branch is a couple months old at this point. Please rebase your changes on the current tip of main. You probably want to squash all your changes first. Once that is done, we can kick off a CI run.

@junpeiz - please re-review these changes.

Done

TobyRoseman · 2024-01-02T20:25:31Z

@alealv - did you forget to git push? I'm still seeing the first commit in your branch that is not from you was from November 14th.

alealv · 2024-01-02T20:51:37Z

@alealv - did you forget to git push? I'm still seeing the first commit in your branch that is not from you was from November 14th.

last commit is from 1 hour ago
1317643

TobyRoseman · 2024-01-03T00:59:26Z

Your most recent commit is not the issue.

The issue is that your first commit (950b1a0) is committed on top of a main commit (7a07062) which is from November 14.

If we were to merge your changes, they would get committed on top of the current tip of main. Your pull request branch is not a good reflection of that, since many changes have been gone into main since November 14.

You basically need to do three things:
1 - Get the most recent changes from upstream main to your fork's main.
2 - Rebase this pull request branch on top of your updated main.
3 - Force push to this branch.

alealv · 2024-01-03T08:28:37Z

Your most recent commit is not the issue.

The issue is that your first commit (950b1a0) is committed on top of a main commit (7a07062) which is from November 14.

If we were to merge your changes, they would get committed on top of the current tip of main. Your pull request branch is not a good reflection of that, since many changes have been gone into main since November 14.

You basically need to do three things: 1 - Get the most recent changes from upstream main to your fork's main. 2 - Rebase this pull request branch on top of your updated main. 3 - Force push to this branch.

Sorry, I was rebasing over origin and not over upstream 😅

TobyRoseman · 2024-01-03T21:43:12Z

This is better. However it's still not right. The most recent commit your branch has from main is now e111123 which is still about five weeks old. Perhaps your upstream is set to somewhere other than this repository.

junpeiz · 2024-01-03T22:28:47Z

It's due to that the author's repo alealv/coremltools that forked from apple/coremltools hasn't been synced. @alealv Could you sync your repo and do rebase? Then it will move your commits based on the most recent head of apple/coremltools.

alealv · 2024-01-04T12:08:58Z

I hope now is correct.

For some reason I had gitlab.com:coremltools1/coremltools as upstream instead of https://github.com/apple/coremltools

TobyRoseman · 2024-01-04T21:49:29Z

You're branch looks good now. Thanks.

Here is the CI run:
https://gitlab.com/coremltools1/coremltools/-/pipelines/1127443513

alealv · 2024-01-05T09:04:41Z

You're branch looks good now. Thanks.

Here is the CI run: https://gitlab.com/coremltools1/coremltools/-/pipelines/1127443513

hmmm, CI is failing con functions I didn't touch.

alealv · 2024-01-09T14:50:35Z

I've dived deep into the CI error.

I didn't understand why it wasn't failing on my machine until I found this condition:

        def test_spectrogram(self, compute_unit, backend, input_shape, spec, power):
            if platform.machine() != "arm64":
                pytest.xfail(
                    "rdar://108001659 ([PyTorch] Torchaudio Spectrogram Failed on Intel Machine)"
                )

Why is this needed?

On arm64 it only fails when power=None

Taking a closer look, if power is None the spectrogram function returns both real and image signals. When power is 1 or 2 then it computes the amplitud or power of the signal. So, I think the problem arise because I miss understood how the DFT matrix was computed. I thought the values were the conjugate of the original but it seems it isn't the case.

My last commit should do the trick. Also, I couldn't find any of the runs of TestSTFT, to verify if those were passing.

junpeiz · 2024-01-10T18:51:51Z

Why is this needed?

When the Spectrogram was implemented, there was a quite strange failure on Intel Machine, and the M chip Machine works fine. We marked it as xfail while we are debugging it.

On arm64 it only fails when power=None. Taking a closer look, if power is None the spectrogram function returns both
real and image signals. When power is 1 or 2 then it computes the amplitud or power of the signal. So, I think the problem arise because I miss understood how the DFT matrix was computed. I thought the values were the conjugate of the original but it seems it isn't the case.

Great investigations! Thanks!

Also, I couldn't find any of the runs of TestSTFT, to verify if those were passing.

Great catch! It's due to that test is marked by @pytest.mark.slow and got skipped (because STFT tests are too slow). I will run it locally for you.

Also, kicked off another CI with your latest commit: https://gitlab.com/coremltools1/coremltools/-/pipelines/1133907719

junpeiz · 2024-01-10T22:42:19Z

@alealv Just want to check if you have run the tests locally on your machine? The pytest coremltools/converters/mil/frontend/torch/test/test_torch_ops.py::TestSTFT::test_istft failed on my local run, with at least following cases failures (I haven't finished a whole run):

         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=None-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=None-length=None-return_complex=True]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=False-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=False-length=None-return_complex=True]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=True-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=None-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=None-length=None-return_complex=True]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=False-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=False-length=None-return_complex=True]

To be safer, let me remove those @pytest.mark.slow decorators and re-run CI: https://gitlab.com/coremltools1/coremltools/-/pipelines/1134078191

alealv · 2024-01-11T19:28:57Z

@alealv Just want to check if you have run the tests locally on your machine? The pytest coremltools/converters/mil/frontend/torch/test/test_torch_ops.py::TestSTFT::test_istft failed on my local run, with at least following cases failures (I haven't finished a whole run):

         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=None-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=None-length=None-return_complex=True]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=False-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=False-length=None-return_complex=True]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=False-onesided=True-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=None-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=None-length=None-return_complex=True]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=False-length=None-return_complex=False]
         - coremltools/converters/mil/frontend/torch/test/test_torch_ops.py:9590 TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=3-n_fft=16-num_frames=9-hop_length=5-win_length=8-window=None-center=True-normalized=True-onesided=False-length=None-return_complex=True]

Yes, all test pass in my Intel machine

Results (28832.46s (8:00:32)):
    21240 passed
    11160 skipped

That's for the whole TestSTFT

junpeiz · 2024-01-11T21:32:04Z

Yes, all test pass in my Intel machine

Nice!

In the most recent run after removing the pytest.slow deco, the M-chip Mac tests failed for iSTFT: https://gitlab.com/coremltools1/coremltools/-/pipelines/1134078191
Could you take a look?

alealv · 2024-01-17T10:08:45Z

The intel tests test_py39_pytorch_intel seems that pytest got a Fatal Python error: Floating point exception... 😕 I don't know how to work with that.
Maybe it entered into some weird state, it would be best if you lunch it once more.

And for the other test_py310_pytorch unfortunately I cannot reproduce.

I ran specifically for the first test case that fails

_ TestSTFT.test_istft[compute_unit=ComputeUnit.CPU_ONLY-backend=('mlprogram', 'fp16')-channels=None-n_fft=16-num_frames=5-hop_length=None-win_length=None-window=None-center=False-normalized=False-onesided=None-length=None-return_complex=False] _

and got

Results (6.42s):
       1 passed

so I guess it's something is done very differently on ARM machines.

junpeiz · 2024-01-17T17:45:43Z

Thank you for trying to reproduce it on your side! Yeah there are a lot of things could lead to different behaviors (OS version, Sys Arch of Intel/ARM, etc). Let me rerun it on CI.

junpeiz reviewed Oct 27, 2023

View reviewed changes

coremltools/converters/mil/frontend/torch/test/test_torch_ops.py Outdated Show resolved Hide resolved

alealv force-pushed the add_istft branch from ab5545d to 8c47c70 Compare November 10, 2023 13:46

junpeiz reviewed Nov 15, 2023

View reviewed changes

coremltools/converters/mil/frontend/torch/test/test_torch_ops.py Outdated Show resolved Hide resolved

junpeiz reviewed Nov 15, 2023

View reviewed changes

coremltools/converters/mil/mil/ops/defs/complex_dialect_ops.py Outdated Show resolved Hide resolved

junpeiz reviewed Nov 15, 2023

View reviewed changes

coremltools/converters/mil/mil/ops/defs/complex_dialect_ops.py Outdated Show resolved Hide resolved

junpeiz reviewed Nov 15, 2023

View reviewed changes

coremltools/converters/mil/mil/passes/defs/lower_complex_dialect_ops.py Outdated Show resolved Hide resolved

junpeiz requested changes Nov 15, 2023

View reviewed changes

alealv force-pushed the add_istft branch 2 times, most recently from 2495a5b to 1aaed1c Compare November 15, 2023 15:47

alealv force-pushed the add_istft branch from 1aaed1c to a85fd81 Compare November 15, 2023 15:51

alealv changed the title ~~Draft: Add istft operation~~ Add istft operation Nov 15, 2023

alealv force-pushed the add_istft branch from a85fd81 to a218ee9 Compare November 15, 2023 15:56

alealv force-pushed the add_istft branch from 1317643 to 44a98ab Compare January 3, 2024 08:27

alealv force-pushed the add_istft branch from f08cf10 to 64e59cf Compare January 4, 2024 12:04

Alejandro Gaston Alvarez Franceschi added 19 commits January 9, 2024 13:42

Normalize by window square

67b5db5

Fix

304b1ca

Add test

ec1c232

Simplify stft

a9e49fd

updates

38aa86b

Try adding length

82eefff

Fix padding

667be1d

Fixes

0bfee9d

Minor

2605f55

Remove ISTFT test class and complex parameter

fe0ad05

Fixes

e0cfffd

More fixes

c2686c8

More fixes

0d3238a

More fixes

4edfd20

More fixes

729d5d4

Fixes

866d61e

Fixes

186110c

More fixes

8cb63ed

More fixes

e016e68

alealv force-pushed the add_istft branch from 64e59cf to e016e68 Compare January 9, 2024 12:43

Seems DFT is the correct one and not the conjugate

c1cf5ec

Removes pytest.mark.slow decorator to run STFT related tests in CI.

6738aba

fwcd mentioned this pull request Jan 24, 2024

Use STFT support in coremltools 7 fwcd/spleeter-pytorch#2

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add istft operation #2029

Add istft operation #2029

alealv commented Oct 24, 2023 •

edited

junpeiz Nov 15, 2023 •

edited

junpeiz left a comment •

edited

alealv commented Nov 15, 2023

alealv commented Nov 15, 2023

junpeiz commented Nov 23, 2023

alealv commented Jan 2, 2024 •

edited

TobyRoseman commented Jan 2, 2024

alealv commented Jan 2, 2024

TobyRoseman commented Jan 2, 2024

alealv commented Jan 2, 2024

TobyRoseman commented Jan 3, 2024

alealv commented Jan 3, 2024 •

edited

TobyRoseman commented Jan 3, 2024

junpeiz commented Jan 3, 2024

alealv commented Jan 4, 2024 •

edited

TobyRoseman commented Jan 4, 2024

alealv commented Jan 5, 2024

alealv commented Jan 9, 2024

junpeiz commented Jan 10, 2024 •

edited

junpeiz commented Jan 10, 2024 •

edited

alealv commented Jan 11, 2024 •

edited

junpeiz commented Jan 11, 2024 •

edited

alealv commented Jan 17, 2024

junpeiz commented Jan 17, 2024

Add istft operation #2029

Are you sure you want to change the base?

Add istft operation #2029

Conversation

alealv commented Oct 24, 2023 • edited

junpeiz Nov 15, 2023 • edited

Choose a reason for hiding this comment

junpeiz left a comment • edited

Choose a reason for hiding this comment

alealv commented Nov 15, 2023

alealv commented Nov 15, 2023

junpeiz commented Nov 23, 2023

alealv commented Jan 2, 2024 • edited

TobyRoseman commented Jan 2, 2024

alealv commented Jan 2, 2024

TobyRoseman commented Jan 2, 2024

alealv commented Jan 2, 2024

TobyRoseman commented Jan 3, 2024

alealv commented Jan 3, 2024 • edited

TobyRoseman commented Jan 3, 2024

junpeiz commented Jan 3, 2024

alealv commented Jan 4, 2024 • edited

TobyRoseman commented Jan 4, 2024

alealv commented Jan 5, 2024

alealv commented Jan 9, 2024

junpeiz commented Jan 10, 2024 • edited

junpeiz commented Jan 10, 2024 • edited

alealv commented Jan 11, 2024 • edited

junpeiz commented Jan 11, 2024 • edited

alealv commented Jan 17, 2024

junpeiz commented Jan 17, 2024

alealv commented Oct 24, 2023 •

edited

junpeiz Nov 15, 2023 •

edited

junpeiz left a comment •

edited

alealv commented Jan 2, 2024 •

edited

alealv commented Jan 3, 2024 •

edited

alealv commented Jan 4, 2024 •

edited

junpeiz commented Jan 10, 2024 •

edited

junpeiz commented Jan 10, 2024 •

edited

alealv commented Jan 11, 2024 •

edited

junpeiz commented Jan 11, 2024 •

edited