API Backend refactor #631

MuawizChaudhary · 2020-07-16T17:55:09Z

This PR is in progress. All the tests currently do not run.

Refactors the named tuple convention out of the 1d.

kymatio/backend/numpy_backend.py

kymatio/backend/tensorflow_backend.py

kymatio/scattering1d/backend/torch_backend.py

tests/scattering1d/test_numpy_backend_1d.py

tests/scattering1d/test_tensorflow_backend_1d.py

tests/scattering1d/test_torch_backend_1d.py

janden · 2020-07-17T02:01:39Z

tests/scattering1d/test_torch_backend_1d.py

@@ -176,6 +177,8 @@ def test_unpad():
    # test unpading of a random tensor
    x = torch.randn(8, 4, 1)

+    Torch1DBackend()


What's going on here?

kymatio/backend/numpy_backend.py

kymatio/backend/tensorflow_backend.py

kymatio/backend/torch_backend.py

kymatio/scattering1d/backend/jax_backend.py

kymatio/scattering1d/backend/numpy_backend.py

tests/scattering1d/test_torch_backend_1d.py

tests/scattering1d/test_torch_scattering1d.py

kymatio/backend/numpy_backend.py

kymatio/backend/tensorflow_backend.py

kymatio/scattering1d/backend/jax_backend.py

kymatio/scattering1d/backend/tensorflow_backend.py

eickenberg · 2020-07-17T17:50:02Z

tests/scattering1d/test_numpy_backend_1d.py

@@ -0,0 +1,117 @@
+import pytest


are these tests from this pr or from the tests refactoring pr? I don't see why the tests would change with this refactoring

This is from the test refactoring PR. We should merge those as soon as possible.

hmm, i believe this is merged, so I'm surprised to see the diff below

MuawizChaudhary

The SKcuda backend is bugging out a bit. in the 'scattering1d/backend/torch_skcuda_backend.py, TorckSKcuda1Ddoesn't properly overrideTorchBackend1D modulus and sub sampling Fourier functions. I've spent some time on it, did some research on the internet, and the functions should be overridden in the 'scattering1d/backend/torch_skcuda_backend.py script, but they aren't.

kymatio/backend/tensorflow_backend.py

eickenberg · 2020-07-20T15:30:19Z

I'd like to discuss today the differences between making changes by subclassing and making changes by passing a constructor argument.

janden · 2020-07-22T20:54:33Z

@MuawizChaudhary Can you rebase this on top of dev? There's a lot of conflicts that need to be resolved and I don't want us to have to review it twice.

MuawizChaudhary · 2020-07-22T21:06:03Z

If you squashed all the changes, I'll re base it shortly!

janden · 2020-07-22T21:19:57Z

If you squashed all the changes, I'll re base it shortly!

The changes in dev from the 1D test PR? Yeah they're squashed.

janden · 2020-07-23T01:48:06Z

Tests fail. Please fix and we'll review.

eickenberg

There are a few things I don't understand (see comments).

Also I am still not 100% convinced we should be using class instances here. Every method I see that takes self as an argument but doesn't use it seems like an unnecessarily class-tied function to me. I would prefer them to be class/static methods.

Can't the whole backend inheritance thing be pulled off without instantiating? Just by using class/static methods and overriding things appropriately?

eickenberg · 2020-07-29T21:44:19Z

kymatio/backend/numpy_backend.py

-    return A * B
+
+class NumpyBackend:
+    def __init__(self, name='numpy'):


Why do we need name as an argument? Aren't we just going to override the __init__ and write the name in there?

We need it so that we can specify whats the name of the backend. We dont have to if its nD numpy backend since we already specified the name here.

Better to override it in the __init__. When would we ever need to specify the name of the backend at instantiation time? Also, what's the point of the name attribute anyhow?

eickenberg · 2020-07-29T21:45:27Z

kymatio/backend/numpy_backend.py

+
+class NumpyBackend:
+    def __init__(self, name='numpy'):
+        import numpy


Please confirm: This is imported inline in order to be able to import this class into another file without triggering a numpy import?

I believe it shouldn't trigger a numpy import unless we call the init functions.

I thought the best idea was to import it inline, I believe that might have been what @janden wanted.

Why is it better to import inline? I figured we would have a global import, but if that's not possible, we can do inline.

Yeah I think nothing is really stopping us from using a global import.

I'd generally also like to get away as much as possible from making an instance. Ideally all functions are static on the class and we use inheritance to pass down those static functions

Yeah I think nothing is really stopping us from using a global import.

Agree. That's how it is for the other backends, as far as I can tell.

I'd generally also like to get away as much as possible from making an instance. Ideally all functions are static on the class and we use inheritance to pass down those static functions

I remember inheriting static functions as being slightly tricky to deal with when it comes to inheritance, but this may be a false memory. Overall, I think it's simple enough to have these singleton instances since it allows us to store some internal state (like these module references).

why can't we just write those into the class?
I'm happy to keep any state that is necessary, but would avoid it if it's easy

kymatio/backend/tensorflow_backend.py

eickenberg · 2020-07-29T21:50:30Z

kymatio/backend/tensorflow_backend.py

-        raise TypeError('The input should be complex.')
+class TensorFlowBackend(NumpyBackend):
+    def __init__(self):
+        super(TensorFlowBackend, self).__init__(name='tensorflow')


It doesn't make sense to me to pass name as an argument. It should be set and overridden in the __init__

the line above?

self.name = 'tensorflow'

kymatio/backend/tensorflow_backend.py

eickenberg · 2020-07-29T21:52:59Z

kymatio/backend/torch_backend.py

+
+        self.contiguous_check(x)
+
+    def complex_check(self, x):


looks like these checks could totally be inherited from a baseclass if we wanted that. not sure if we do

I feel like this is a smart idea.

I guess this is the base class actually.
But we might be able to use one more level of inheritance to deal with the two different complex number formulations

eickenberg · 2020-07-29T21:54:44Z

kymatio/backend/torch_skcuda_backend.py

-        cublas.cublasSetStream(handle, stream)
-        cublas.cublasCdgmm(handle, 'l', m, n, A.data_ptr(), lda, B.data_ptr(), incx, C.data_ptr(), ldc)
-        return C
+class TorchSKcudaBackend:


But this one I would have expected to inherit from the torch backend

we inherit the torch backend from the 1d torch backend when we create the TorchSKcuda 1D backend class. The torch skcuda base backend is also inherited.

I recommend checking kymatio/scattering1d/backend/torch_skcuda_backend.py.

@eickenberg, same thing happens for Jax (NP → NP1D → Jax1D) since we need to inherit the 1D-specific functions. Ideally, in the future, the backends will be less dimension-specific and we can have a simpler inheritance structure. Perhaps mixins could simplify the 1D/2D/3D specification when needed.

On another, I suggest TorchSkcudaBackend. Much less damaging to the eyes.

eickenberg · 2020-07-29T21:57:28Z

kymatio/scattering1d/backend/numpy_backend.py

-def irfft(x):
-    complex_check(x)
-    return scipy.fftpack.ifft(x).real
+        y = x.reshape(-1, k, x.shape[-1] // k)


I would add the initial shape back. There is absolutely no reason not to.

y = x.reshape(x.shape[:-1] + (k, x.shape[-1] // k)).sum(-2)

OK, this part, when I implement it as such, messes with a reshape downstream. I can't properly figure it out, so will leave this as is for now.

kymatio/scattering1d/backend/numpy_backend.py

kymatio/scattering1d/backend/tensorflow_backend.py

janden

Looks good. Just a few things to address, then we can extend this to 2D and 3D.

janden · 2020-08-11T18:34:00Z

kymatio/backend/numpy_backend.py

-    return A * B
+
+class NumpyBackend:
+    def __init__(self, name='numpy'):


Better to override it in the __init__. When would we ever need to specify the name of the backend at instantiation time? Also, what's the point of the name attribute anyhow?

janden · 2020-08-11T18:34:54Z

kymatio/backend/numpy_backend.py

+
+class NumpyBackend:
+    def __init__(self, name='numpy'):
+        import numpy


Why is it better to import inline? I figured we would have a global import, but if that's not possible, we can do inline.

janden · 2020-08-11T18:44:22Z

kymatio/backend/tensorflow_backend.py

-        raise TypeError('The input should be complex.')
+class TensorFlowBackend(NumpyBackend):
+    def __init__(self):
+        super(TensorFlowBackend, self).__init__(name='tensorflow')


self.name = 'tensorflow'

kymatio/backend/tensorflow_backend.py

janden · 2020-08-11T18:51:56Z

kymatio/backend/torch_backend.py

-    else:
-        contiguous_check(B)
+    def concatenate(self, arrays):
+        return torch.stack(arrays, dim=2)


Why not dim=1 like in the NP backend?

kymatio/scattering1d/backend/numpy_backend.py

janden · 2020-08-11T19:05:58Z

tests/general/test_torch_backend.py



 def test_modulus(random_state=42):
    """
    Tests the stability and differentiability of modulus
    """
-
+    backend = TorchBackend()


Why not just import backend from kymatio.backend.torch_backend? It's already been instantiated and that's what the main code will use.

especially now, since it's actually not instantiated

though that said, it might be more modular to do it like above, just without the parentheses

janden · 2020-08-11T19:06:34Z

tests/scattering1d/test_tensorflow_backend_1d.py

@@ -1,6 +1,6 @@
 import pytest
 import numpy as np
-
+import tensorflow as tf


Why do we need this?

i removed it and it still works, so no reason to keep it

janden · 2020-08-11T19:07:08Z

tests/scattering1d/test_tensorflow_backend_1d.py

@@ -106,14 +106,11 @@ def coefficent(n):
    y_r = (x_r * coefficents).sum(-1)

    z = backend.rfft(x_r)
-    # increase tolerance here as tensorflow fft is slightly inaccurate due to


Why is this going away?

tests/scattering1d/test_torch_backend_1d.py

eickenberg · 2021-02-06T02:11:02Z

OK, I just rebased this.

I will need some help to figure out what the intention was in some of these changes.

The global picture is that we are introducing classes for backends instead of the named tuples.

Please take a look at the changes I needed to make in my first commit on top of this to make the tests for numpy backend 2d pass: 8ac8a8b

basically I had to write a bunch of attributes to the instantiated class. I left the original commented for comparison.

Question: Why is it that we are defining half of the backend in the class and half in the 2d backend file? Is it because those parts are the specific stuff? Should we potentially be doing this using inheritance?

Happy for any input and opinions @janden @MuawizChaudhary

eickenberg · 2021-02-09T02:55:18Z

OK, I have kludged my way to making the tests pass.

For the specific backends I would suggest using inheritance rather than writing properties, since that is not much better than the named tuples we had before.

Please let me know if you think that makes sense and I'll make those changes. Ideally we get this one merged before the sprint (or at the latest at the beginning of it).

janden · 2021-02-10T09:09:41Z

Still fails, but for legit reasons (torch_skcuda backend has not been updated to the new architecture).

janden · 2021-02-10T18:39:18Z

So tests pass now on Jenkins. Since it's been a while, I want to go over everything again to see what needs to be done before merging.

eickenberg · 2021-02-10T23:45:24Z

Awesome. I will read through the updated code now.
One thing that I have had simmering for a bit is the instantiation. Do we really need to instantiate these backends?
If not, we could set all the methods to staticmethod and remove the selfobjects because they are not needed

edouardoyallon · 2021-02-11T07:45:09Z

the fft failure for python 3.6 only, with conda and tf sounds like some future promising fun!

eickenberg · 2021-02-11T17:38:50Z

kymatio/backend/torch_backend.py

+        self.complex_check(x)
+        self.contiguous_check(x)
+
+    def contiguous_check(self, x):


yeah I'm not happy with these unused self objects

eickenberg · 2021-02-11T17:50:40Z

@janden is it OK if I go through the backends and remove any unnecessary use of selfin function definitions? I.e. functions that do not depend on state should not need a self object as input

janden · 2021-02-11T22:21:07Z

@eickenberg So all the backends have been refactored now to have either class methods or stack methods. There are no object instantiations (as far as I can tell). Please have a look and let me know what you think.

eickenberg

This looks close to mergeable. At this point I think if we address all the remaining comments, we should be good to go

eickenberg · 2021-02-12T00:33:42Z

kymatio/backend/tensorflow_backend.py

-    return A * B
-
+    @classmethod
+    def cdgmm(cls, A, B):


This method is identical to the one in the numpy backend so should go through by inheritance

eickenberg · 2021-02-12T01:00:20Z

kymatio/scattering3d/backend/tensorflow_backend.py

-backend.compute_integrals = compute_integrals
-backend.concatenate = concatenate
+
+backend = TensorFlowBackend3D()


instantiation not necessary, correct?

eickenberg · 2021-02-12T01:01:39Z

tests/general/test_torch_backend.py



 def test_modulus(random_state=42):
    """
    Tests the stability and differentiability of modulus
    """
-
+    backend = TorchBackend()


especially now, since it's actually not instantiated

eickenberg · 2021-02-12T01:02:29Z

tests/general/test_torch_backend.py



 def test_modulus(random_state=42):
    """
    Tests the stability and differentiability of modulus
    """
-
+    backend = TorchBackend()


though that said, it might be more modular to do it like above, just without the parentheses

eickenberg · 2021-02-12T01:03:18Z

tests/scattering1d/test_numpy_backend_1d.py

@@ -0,0 +1,117 @@
+import pytest


hmm, i believe this is merged, so I'm surprised to see the diff below

eickenberg

OK, I think I'm done for today.

I have addressed a number of outstanding code comments in the PR.

The remaining ones I am not sure how to address, but I have the feeling they are minor.

I did an interactive rebase in order to consolidate @MuawizChaudhary 's summer work with my commits from last week so that in aggregate the tests pass on that commit. Just in case this wasn't a good idea, I have the unsquashed branch fully backed up on my computer. You can also reconstruct the previous status quo by using what you have on your machine and adding my most recent commits on top.

There is one remaining puzzle: I found (and had commented on in the PR at some point) several changes that looked like they were from a previous test refactoring project. I had thought that these might be duplicates, but for most of them, I can't find a trace of a second version of them anywhere in the history. If I am right, then we should make sure to keep these commits, because they do work on the tests.

From my side this can be merged as is, since merging will just add the test commits, and the remaining comments are minor. However, it is also possible to address the comments and split off the TST commits in order to make this PR only about the backend refactor.

I am fine with either. Since it's the last sprint day tomorrow, maybe we can get this in and then try to add the jax backends?

eickenberg · 2021-02-12T04:25:32Z

kymatio/scattering1d/backend/numpy_backend.py

-def irfft(x):
-    complex_check(x)
-    return scipy.fftpack.ifft(x).real
+        y = x.reshape(-1, k, x.shape[-1] // k)


OK, this part, when I implement it as such, messes with a reshape downstream. I can't properly figure it out, so will leave this as is for now.

eickenberg · 2021-02-12T04:51:04Z

tests/scattering1d/test_tensorflow_backend_1d.py

@@ -1,6 +1,6 @@
 import pytest
 import numpy as np
-
+import tensorflow as tf


i removed it and it still works, so no reason to keep it

eickenberg · 2021-02-13T01:02:08Z

OK, as discussed with @janden, I squashed all the straggling TST commits together and hilariously all those commits boiled down to these very few lines removing the tolerance from tensorflow fft test a6dee63

I will now proceed to remove this commit, because we don't actually want that change.

This is a squash of many commits. Muawiz had started this project as part of his summer internship. In order to finish it and to make sure the commits in the git history pass tests we aggregate them here because taken separately they would not pass tests because the commits contain partial reorganization. API adding refactor backends MAINT include backend initalization at the end of each file. renamed each backend class MAINT skcuda backend refactored MAINT removed pass in torch backend MAINT removal of jax backend MAINT removed redundant line STY white space added and removed STY white space changes MAINT removed BACKEND_NAME variable MAINT added checks back into numpy backend MAINT refactored checks back into torch backends MAINT checks added back into tensorflow backend STY removed parenthesis MAINT fixed incorrect modulus call MAINT add option to specify name for numpy backend MAINT removed inplace operation, fixed torck_skcuda calling torch backend functions STY removed parenthesis STY remove parenthesis MAINT move modulus inside torch class MAINT remove initalization of checks MAINT define concatenate function in general backend MAINT define concatenate function in general backend tensorflow MAINT define concatenate function in general backend numpy MAINT do numpy imports in backends MAINT tensorflow now inheriates from numpy backend TST changed error message we check for to appropriate one. MAINT moved checks to top, moved concatenate above. TST general backend test TST remove print statement FIX make tests pass for numpy backend 2D MAINT make tests pass for all backends. Next step is probably to create subclasses for the different-dimensional backends Co-authored-by: chaudhm <chaudhm@wwu.edu> Co-authored-by: Joakim Andén <janden@kth.se>

MAINT Fix spelling of Skcuda backend class TST Fix bug in 2D torch subsample tests MAINT Update 2D Torch backend to use inheritance MAINT Make torch_skcuda backend in 2D use inheritance MAINT Make 3D torch backend use inheritance MAINT Make 3D torch_skcuda backend use inheritance FIX remove backend name as class init argument ENH numpy 2d backend class COSM remove commented code ENH tensorflow 2d backend class ENH numpy 3d backend class ENH tensorflow 3d backend class MAINT Update NP backends with class methods MAINT Update TF backends with class methods MAINT Update Torch backends to use class methods MAINT remove cdgmm from tf backend because inherited from numpy backend COSM remove commented code MAINT make tf 3d backend use static and class methods MAINT remove instantiation from torch backend test MAINT remove tf import from tf 1d backend test Co-authored-by: Joakim Andén <janden@kth.se>

these are change requests from july that have been addressed

janden · 2021-02-15T08:27:23Z

👍

MuawizChaudhary requested review from eickenberg and janden July 16, 2020 17:55

janden requested changes Jul 17, 2020

View reviewed changes

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/backend/numpy_backend.py Outdated Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/backend/numpy_backend.py Outdated Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/backend/tensorflow_backend.py Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/backend/tensorflow_backend.py Outdated Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/scattering1d/backend/jax_backend.py Outdated Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/scattering1d/backend/tensorflow_backend.py Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

kymatio/scattering1d/backend/tensorflow_backend.py Outdated Show resolved Hide resolved

eickenberg reviewed Jul 17, 2020

View reviewed changes

MuawizChaudhary commented Jul 18, 2020

View reviewed changes

kymatio/backend/tensorflow_backend.py Outdated Show resolved Hide resolved

MuawizChaudhary requested review from janden and eickenberg July 22, 2020 16:54

MuawizChaudhary force-pushed the backend_refactor branch from 546fc9a to fd32e49 Compare July 22, 2020 22:56

eickenberg reviewed Jul 29, 2020

View reviewed changes

janden previously requested changes Aug 11, 2020

View reviewed changes

eickenberg force-pushed the backend_refactor branch from 2e2cbc3 to 8ac8a8b Compare February 6, 2021 02:05

janden force-pushed the backend_refactor branch from dd48f5f to aa5efa7 Compare February 10, 2021 09:03

janden force-pushed the backend_refactor branch 2 times, most recently from 35ae750 to 0738934 Compare February 10, 2021 16:00

janden mentioned this pull request Feb 10, 2021

ENH backend should be classes and not namedtuple #452

Closed

edouardoyallon self-assigned this Feb 11, 2021

eickenberg reviewed Feb 11, 2021

View reviewed changes

eickenberg reviewed Feb 12, 2021

View reviewed changes

eickenberg force-pushed the backend_refactor branch from ab660d8 to 1719eb0 Compare February 12, 2021 05:19

eickenberg approved these changes Feb 12, 2021

View reviewed changes

eickenberg force-pushed the backend_refactor branch from 1719eb0 to e5e4783 Compare February 13, 2021 00:58

MuawizChaudhary and others added 2 commits February 12, 2021 20:05

eickenberg force-pushed the backend_refactor branch from e5e4783 to ce74429 Compare February 13, 2021 01:06

eickenberg merged commit c43e498 into kymatio:dev Feb 13, 2021

API Backend refactor #631

API Backend refactor #631

Conversation

MuawizChaudhary commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuawizChaudhary left a comment

Choose a reason for hiding this comment

eickenberg commented Jul 20, 2020

janden commented Jul 22, 2020

MuawizChaudhary commented Jul 22, 2020

janden commented Jul 22, 2020

janden commented Jul 23, 2020

eickenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eickenberg commented Feb 6, 2021

eickenberg commented Feb 9, 2021

janden commented Feb 10, 2021

janden commented Feb 10, 2021

eickenberg commented Feb 10, 2021

edouardoyallon commented Feb 11, 2021

Choose a reason for hiding this comment

eickenberg commented Feb 11, 2021

janden commented Feb 11, 2021

eickenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eickenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eickenberg commented Feb 13, 2021

janden commented Feb 15, 2021