Repository: kymatio/kymatio

Issue:
redefine 1D `self.N`, deprecate`self.J_pad`?
in 1D, `self.N` is the unpadded length of the input signal. It is defined in `ScatteringBase1D` as some derivative of `self.shape` and isn't being used anywhere else. 

Same for 2D where `self.shape` unpacks as `self.M`, `self.N`. 

I see no reason to keep these values as attributes in the object when:
* we have `self.shape` for that
* these values are only being used transiently, in `self.build()`
* our filterbank construction relies on `N_padded`, not `N`
* these values are trivially available in the forward via `self.N = len(x)`

In the spirit of preparing for scattering on variable-length inputs (`shape=None`, a long-requested feature !), i ask the permission to remove these attributes from `self`.

I am open to making a deprecation cycle where we convert these attributes to read-only properties and print a `DeprecationWarning` in v0.3 if anyone tries to access these numbers (NB: none of our examples access `self.N` and friends)

@janden @eickenberg 

Discussion:
lostanlen: next up, fix the newly introduced `DeprecationWarning` in `scattering_filter_factory` !
lostanlen: i have reduced the PR size. this runtime shape check i experimented with should happen in a separate PR, ideally a backend agnostic one
codecov-commenter: # [Codecov](https://codecov.io/gh/kymatio/kymatio/pull/863?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio) Report
> Merging [#863](https://codecov.io/gh/kymatio/kymatio/pull/863?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio) (3ef38fd) into [dev](https://codecov.io/gh/kymatio/kymatio/commit/4a6d9eaa691351c001c0b751ed3426b97ee36367?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio) (4a6d9ea) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head 3ef38fd differs from pull request most recent head 4dc679e. Consider uploading reports for the commit 4dc679e to get more accurate results

```diff
@@            Coverage Diff             @@
##              dev     #863      +/-   ##
==========================================
+ Coverage   87.83%   87.87%   +0.03%     
==========================================
  Files          64       64              
  Lines        2253     2260       +7     
==========================================
+ Hits         1979     1986       +7     
  Misses        274      274              
```

| Flag | Coverage Δ | |
|---|---|---|
| jenkins_main | `87.87% <100.00%> (+0.03%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/kymatio/kymatio/pull/863?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio) | Coverage Δ | |
|---|---|---|
| [kymatio/scattering1d/frontend/base\_frontend.py](https://codecov.io/gh/kymatio/kymatio/pull/863/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio#diff-a3ltYXRpby9zY2F0dGVyaW5nMWQvZnJvbnRlbmQvYmFzZV9mcm9udGVuZC5weQ==) | `97.61% <100.00%> (+0.18%)` | :arrow_up: |
| [kymatio/scattering1d/frontend/numpy\_frontend.py](https://codecov.io/gh/kymatio/kymatio/pull/863/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio#diff-a3ltYXRpby9zY2F0dGVyaW5nMWQvZnJvbnRlbmQvbnVtcHlfZnJvbnRlbmQucHk=) | `72.22% <100.00%> (+0.79%)` | :arrow_up: |
| [...matio/scattering1d/frontend/tensorflow\_frontend.py](https://codecov.io/gh/kymatio/kymatio/pull/863/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio#diff-a3ltYXRpby9zY2F0dGVyaW5nMWQvZnJvbnRlbmQvdGVuc29yZmxvd19mcm9udGVuZC5weQ==) | `72.97% <100.00%> (+0.75%)` | :arrow_up: |
| [kymatio/scattering1d/utils.py](https://codecov.io/gh/kymatio/kymatio/pull/863/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio#diff-a3ltYXRpby9zY2F0dGVyaW5nMWQvdXRpbHMucHk=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kymatio/kymatio/pull/863?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kymatio/kymatio/pull/863?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio). Last update [4a6d9ea...4dc679e](https://codecov.io/gh/kymatio/kymatio/pull/863?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kymatio).

lostanlen: seems like i've broken everything with this commit. i can't even run the tests anymore 🤕 
janden: Looks like the OOM killer strikes again: https://stackoverflow.com/questions/68844666/github-action-is-being-killed…
lostanlen: Facepalm. I guess building a filterbank of support `2**(2**J)` wasn't too responsible of me
lostanlen: Alright, ready now. I'm waiting for the codecov report and then we can consider merging.
Some open PRs will need a rebase
lostanlen: i'm making a public service announcement (extended to @cyrusvahidi @changhongw) because this PR has an important change of semantics. In your code:

* replace `self.N` by `self.shape[0]` (input length)
* replace `2**self.J_pad` by `len(self.phi[0])` (filter length)
lostanlen: rebased. can i get approval from either @changhongw or @cyrusvahidi ?
cyrusvahidi: > rebased. can i get approval from either @changhongw or @cyrusvahidi ?

LGTM
lostanlen: @MuawizChaudhary i think @janden already approved the overall idea, and we talked about that during the meeting yesterday. I think i can speak for him that either you or @cyrusvahidi are free to merge it
cyrusvahidi: > @MuawizChaudhary i think @janden already approved the overall idea, and we talked about that during the meeting yesterday. I think i can speak for him that either you or @cyrusvahidi are free to merge it

can you resolve the conflicts?
lostanlen: done
lostanlen: Rebased from #854, which was relying on the old semantics (`2**J_scattering`)

@MuawizChaudhary please, can we make this PR next in line for merging?
MuawizChaudhary: The pull request fulfills the specifications laid out by @lostanlen and @janden in issue #857 . Therefore I am merging.

If there are any complaints about the design proposed here, please make an issue or discuss it during our next weekly meeting. 

Patch:
diff --git a/kymatio/scattering1d/filter_bank.py b/kymatio/scattering1d/filter_bank.py
index 003cfba..090e48d 100644
--- a/kymatio/scattering1d/filter_bank.py
+++ b/kymatio/scattering1d/filter_bank.py
@@ -287,7 +287,7 @@ def compute_params_filterbank(sigma_min, Q, alpha, r_psi=math.sqrt(0.5)):
     ----------
     sigma_min : float
         This acts as a lower bound on the frequential widths of the band-pass
-        filters. The low-pass filter may be wider (if T < 2**J_scattering), making
+        filters. The low-pass filter may be wider (if T < _N_padded), making
         invariants over shorter time scales than longest band-pass filter.
     Q : int
         number of wavelets per octave.
@@ -337,7 +337,7 @@ def compute_params_filterbank(sigma_min, Q, alpha, r_psi=math.sqrt(0.5)):
     return xis, sigmas, js
 
 
-def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5),
+def scattering_filter_factory(N, J, Q, T, r_psi=math.sqrt(0.5),
                               max_subsampling=None, sigma0=0.1, alpha=5., **kwargs):
     """
     Builds in Fourier the Morlet filters used for the scattering transform.
@@ -354,11 +354,6 @@ def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5
 
     Parameters
     ----------
-    J_support : int
-        2**J_support is the desired support size of the filters
-    J_scattering : int
-        parameter for the scattering transform (2**J_scattering
-        corresponds to maximal temporal support of any filter)
     Q : tuple
         number of wavelets per octave at the first and second order 
         Q = (Q1, Q2). Q1 and Q2 are both int >= 1.
@@ -374,9 +369,8 @@ def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5
         to save computation time if it is not required. Defaults to None, in
         which case this value is dynamically adjusted depending on the filters.
     sigma0 : float, optional
-        parameter controlling the frequential width of the
-        low-pass filter at J_scattering=0; at a an absolute J_scattering, it
-        is equal to sigma0 / 2**J_scattering. Defaults to 1e-1
+        parameter controlling the frequential width of the low-pass filter at
+        j=0; at a an absolute J, it is equal to sigma0 / 2**J. Defaults to 0.1
     alpha : float, optional
         tolerance factor for the aliasing after subsampling.
         The larger alpha, the more conservative the value of maximal
@@ -416,7 +410,7 @@ def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5
     https://tel.archives-ouvertes.fr/tel-01559667
     """
     # compute the spectral parameters of the filters
-    sigma_min = sigma0 / math.pow(2, J_scattering)
+    sigma_min = sigma0 / math.pow(2, J)
     Q1, Q2 = Q
     xi1s, sigma1s, j1s = compute_params_filterbank(sigma_min, Q1, alpha, r_psi)
     xi2s, sigma2s, j2s = compute_params_filterbank(sigma_min, Q2, alpha, r_psi)
@@ -435,8 +429,7 @@ def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5
         # compute the current value for the max_subsampling,
         # which depends on the input it can accept.
         if max_subsampling is None:
-            possible_subsamplings_after_order1 = [
-                j1 for j1 in j1s if j2 > j1]
+            possible_subsamplings_after_order1 = [j1 for j1 in j1s if j2 > j1]
             if len(possible_subsamplings_after_order1) > 0:
                 max_sub_psi2 = max(possible_subsamplings_after_order1)
             else:
@@ -444,7 +437,6 @@ def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5
         else:
             max_sub_psi2 = max_subsampling
         # We first compute the filter without subsampling
-        N = 2**J_support
 
         psi_levels = [morlet_1d(N, xi2, sigma2)]
         # compute the filter after subsampling at all other subsamplings
@@ -457,7 +449,6 @@ def scattering_filter_factory(J_support, J_scattering, Q, T, r_psi=math.sqrt(0.5
     # for the 1st order filters, the input is not subsampled so we
     # can only compute them with N=2**J_support
     for (xi1, sigma1, j1) in zip(xi1s, sigma1s, j1s):
-        N = 2**J_support
         psi_levels = [morlet_1d(N, xi1, sigma1)]
         psi1_f.append({'levels': psi_levels, 'xi': xi1, 'sigma': sigma1, 'j': j1})
 
diff --git a/kymatio/scattering1d/frontend/base_frontend.py b/kymatio/scattering1d/frontend/base_frontend.py
index 436cd40..a20059d 100644
--- a/kymatio/scattering1d/frontend/base_frontend.py
+++ b/kymatio/scattering1d/frontend/base_frontend.py
@@ -1,8 +1,8 @@
 from ...frontend.base_frontend import ScatteringBase
 import math
 import numbers
-
 import numpy as np
+from warnings import warn
 
 from ..filter_bank import compute_temporal_support, gauss_1d, scattering_filter_factory
 from ..utils import (compute_border_indices, compute_padding,
@@ -52,46 +52,48 @@ class ScatteringBase1D(ScatteringBase):
         else:
             raise ValueError("Q must be an integer or a tuple")
 
-        # check the shape
+        # check input length
         if isinstance(self.shape, numbers.Integral):
-            self.N = self.shape
+            self.shape = (self.shape,)
         elif isinstance(self.shape, tuple):
-            self.N = self.shape[0]
             if len(self.shape) > 1:
                 raise ValueError("If shape is specified as a tuple, it must "
                                  "have exactly one element")
         else:
             raise ValueError("shape must be an integer or a 1-tuple")
+        N_input = self.shape[0]
 
         # check T or set default
         if self.T is None:
             self.T = 2**(self.J)
-        elif self.T > self.N:
+        elif self.T > N_input:
             raise ValueError("The temporal support T of the low-pass filter "
                              "cannot exceed input length (got {} > {})".format(
-                                 self.T, self.N))
+                                 self.T, N_input))
         self.log2_T = math.floor(math.log2(self.T))
 
         # Compute the minimum support to pad (ideally)
-        phi_f = gauss_1d(self.N, self.sigma0/self.T)
+        phi_f = gauss_1d(N_input, self.sigma0/self.T)
         min_to_pad = 3 * compute_temporal_support(
             phi_f.reshape(1, -1), criterion_amplitude=1e-3)
 
         # to avoid padding more than N - 1 on the left and on the right,
         # since otherwise torch sends nans
-        J_max_support = int(np.floor(np.log2(3 * self.N - 2)))
-        self.J_pad = min(int(np.ceil(np.log2(self.N + 2 * min_to_pad))),
-                         J_max_support)
+        J_max_support = int(np.floor(np.log2(3 * N_input - 2)))
+        J_pad = min(int(np.ceil(np.log2(N_input + 2 * min_to_pad))),
+                    J_max_support)
+        self._N_padded = 2**J_pad
+
         # compute the padding quantities:
-        self.pad_left, self.pad_right = compute_padding(self.J_pad, self.N)
+        self.pad_left, self.pad_right = compute_padding(self._N_padded, N_input)
         # compute start and end indices
         self.ind_start, self.ind_end = compute_border_indices(
-            self.log2_T, self.J, self.pad_left, self.pad_left + self.N)
+            self.log2_T, self.J, self.pad_left, self.pad_left + N_input)
 
     def create_filters(self):
         # Create the filters
         self.phi_f, self.psi1_f, self.psi2_f = scattering_filter_factory(
-            self.J_pad, self.J, self.Q, self.T,
+            self._N_padded, self.J, self.Q, self.T,
             r_psi=self.r_psi, sigma0=self.sigma0, alpha=self.alpha)
 
     def meta(self):
@@ -147,6 +149,20 @@ class ScatteringBase1D(ScatteringBase):
                 'Input tensor x should have at least one axis, got {}'.format(
                     len(x.shape)))
 
+    @property
+    def J_pad(self):
+        warn("The attribute J_pad is deprecated and will be removed in v0.4. "
+        "Measure len(self.phi_f[0]) for the padded length (previously 2**J_pad) "
+        "or access shape[0] for the unpadded length (previously N).", DeprecationWarning)
+        return int(np.log2(self._N_padded))
+
+    @property
+    def N(self):
+        warn("The attribute N is deprecated and will be removed in v0.4. "
+        "Measure len(self.phi_f[0]) for the padded length (previously 2**J_pad) "
+        "or access shape[0] for the unpadded length (previously N).", DeprecationWarning)
+        return int(self.shape[0])
+
     _doc_shape = 'N'
 
     _doc_instantiation_shape = {True: 'S = Scattering1D(J, N, Q)',
@@ -158,9 +174,7 @@ class ScatteringBase1D(ScatteringBase):
         """
 
     _doc_attrs_shape = \
-    r"""J_pad : int
-            The logarithm of the padded length of the signals.
-        pad_left : int
+    r"""pad_left : int
             The amount of padding to the left of the signal.
         pad_right : int
             The amount of padding to the right of the signal.
diff --git a/kymatio/scattering1d/utils.py b/kymatio/scattering1d/utils.py
index 6261436..6bddf3e 100644
--- a/kymatio/scattering1d/utils.py
+++ b/kymatio/scattering1d/utils.py
@@ -39,33 +39,32 @@ def compute_border_indices(log2_T, J, i0, i1):
         ind_end[j] = (ind_end[j - 1] // 2) + (ind_end[j - 1] % 2)
     return ind_start, ind_end
 
-def compute_padding(J_pad, N):
+def compute_padding(N, N_input):
     """
     Computes the padding to be added on the left and on the right
     of the signal.
 
-    It should hold that 2**J_pad >= N
+    It should hold that N >= N_input
 
     Parameters
     ----------
-    J_pad : int
-        2**J_pad is the support of the padded signal
     N : int
-        original signal support size
+        support of the padded signal
+    N_input : int
+        support of the unpadded signal
 
     Returns
     -------
     pad_left: amount to pad on the left ("beginning" of the support)
     pad_right: amount to pad on the right ("end" of the support)
     """
-    N_pad = 2**J_pad
-    if N_pad < N:
+    if N < N_input:
         raise ValueError('Padding support should be larger than the original' +
                          'signal size!')
-    to_add = 2**J_pad - N
+    to_add = N - N_input
     pad_left = to_add // 2
     pad_right = to_add - pad_left
-    if max(pad_left, pad_right) >= N:
+    if max(pad_left, pad_right) >= N_input:
         raise ValueError('Too large padding value, will lead to NaN errors')
     return pad_left, pad_right
 
@@ -95,9 +94,8 @@ def precompute_size_scattering(J, Q, T, max_order, r_psi, sigma0, alpha):
         Should be >0 and <1. Controls the redundancy of the filters
         (the larger r_psi, the larger the overlap between adjacent wavelets).
     sigma0 : float
-        parameter controlling the frequential width of the
-        low-pass filter at J_scattering=0; at a an absolute J_scattering, it
-        is equal to sigma0 / 2**J_scattering.
+        parameter controlling the frequential width of the low-pass filter at
+        j=0; at a an absolute J, it is equal to sigma0 / 2**J.
     alpha : float, optional
         tolerance factor for the aliasing after subsampling.
         The larger alpha, the more conservative the value of maximal
@@ -150,9 +148,8 @@ def compute_meta_scattering(J, Q, T, max_order, r_psi, sigma0, alpha):
         Should be >0 and <1. Controls the redundancy of the filters
         (the larger r_psi, the larger the overlap between adjacent wavelets).
     sigma0 : float
-        parameter controlling the frequential width of the
-        low-pass filter at J_scattering=0; at a an absolute J_scattering, it
-        is equal to sigma0 / 2**J_scattering.
+        parameter controlling the frequential width of the low-pass filter at
+        j=0; at a an absolute J, it is equal to sigma0 / 2**J.
     alpha : float, optional
         tolerance factor for the aliasing after subsampling.
         The larger alpha, the more conservative the value of maximal


Test patch:
diff --git a/tests/scattering1d/test_numpy_scattering1d.py b/tests/scattering1d/test_numpy_scattering1d.py
index 9de28cb..042a050 100644
--- a/tests/scattering1d/test_numpy_scattering1d.py
+++ b/tests/scattering1d/test_numpy_scattering1d.py
@@ -96,7 +96,7 @@ class TestScattering1DNumpy:
                     default_str = ' (default)'
                 else:
                     default_str = ''
-                phi_f, psi1_f, psi2_f = scattering_filter_factory(np.log2(N), J, Q, T)
+                phi_f, psi1_f, psi2_f = scattering_filter_factory(N, J, Q, T)
                 assert(phi_f['sigma']==0.1/T)
 
 frontends = ['numpy', 'sklearn']
diff --git a/tests/scattering1d/test_utils_scattering1d.py b/tests/scattering1d/test_utils_scattering1d.py
index 3ff8235..2bbd33c 100644
--- a/tests/scattering1d/test_utils_scattering1d.py
+++ b/tests/scattering1d/test_utils_scattering1d.py
@@ -10,15 +10,15 @@ def test_compute_padding():
     Test the compute_padding function
     """
 
-    pad_left, pad_right = compute_padding(5, 16)
+    pad_left, pad_right = compute_padding(32, 16)
     assert pad_left == 8 and pad_right == 8
 
     with pytest.raises(ValueError) as ve:
-        _, _ = compute_padding(3, 16)
+        _, _ = compute_padding(8, 16)
     assert "should be larger" in ve.value.args[0]
 
     with pytest.raises(ValueError) as ve:
-        _, _ = compute_padding(6, 16)
+        _, _ = compute_padding(64, 16)
     assert "Too large padding value" in ve.value.args[0]