MAINT deprecate `self.N` and `self.J_pad` in 1D by lostanlen · Pull Request #863 · kymatio/kymatio

lostanlen · 2022-06-09T23:29:11Z

Fixes #857

An important deprecation to set before we release v0.3 is J_pad. Indeed, there won't be a J_pad_fr in JTFS, so it'd best to plan for symmetry in design.

_N_padded, _N_padded_fr
J, J_fr
Q, Q_fr
T, F
log2_T, log2_F

I have moved the shape check to its own static method. We'll be able to call this method a second time inside self._check_input(x) (recently merged!)

MuawizChaudhary

I approve.

lostanlen · 2022-06-09T23:43:17Z

next up, fix the newly introduced DeprecationWarning in scattering_filter_factory !

_get_shapes was not a good idea

lostanlen · 2022-06-11T06:00:12Z

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 · 2022-06-11T06:41:45Z

Codecov Report

Merging #863 (6c1168c) into dev (a776d57) will decrease coverage by 0.14%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##              dev     #863      +/-   ##
==========================================
- Coverage   88.22%   88.08%   -0.15%     
==========================================
  Files          64       64              
  Lines        2242     2249       +7     
==========================================
+ Hits         1978     1981       +3     
- Misses        264      268       +4

Flag	Coverage Δ
jenkins_main	`88.08% <83.33%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
kymatio/scattering1d/frontend/base_frontend.py	`92.92% <76.47%> (-3.70%)`	⬇️
kymatio/scattering1d/filter_bank.py	`94.39% <100.00%> (-0.11%)`	⬇️
kymatio/scattering1d/utils.py	`100.00% <100.00%> (ø)`

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 a776d57...6c1168c. Read the comment docs.

MuawizChaudhary

Looking to @janden for a final approval.

lostanlen · 2022-06-16T15:23:19Z

@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

cyrusasfa · 2022-06-17T14:38:59Z

@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 · 2022-06-17T14:42:15Z

done

into redefine-N-1D-bis

lostanlen · 2022-06-17T14:56:20Z

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 · 2022-06-18T10:21:50Z

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.

janden

Looks good but we're now missing docstring for N, J in scattering_filter_factory.

janden · 2022-06-20T11:53:00Z

Will make an issue.

lostanlen · 2022-06-20T12:24:41Z

oops! yes good catch, needs a new doc now.

thanks for merging 🙏🏻

As a consequence of kymatio#863.

* FIX Update examples/1d/plot_filters.py As a consequence of #863. * FIX Update examples/2d/plot_filters Add new levels key.

* redefine self.N, deprecate self.J_pad * deprecationwarning * restore self.vectorize (temporarily) * bugfix _get_input_length * J_pad = int(np.log2(self.N)) * bugfix _check_input * revert to _check_input, write _get_shapes * update _get_shapes * try calling self._get_shapes in frontends * shape_fn in _get_shapes * write tensorflow shape_fn * move _check_input into _get_shapes * remove self._check_input * bugfix tensorflow shape check * reduce PR size _get_shapes was not a good idea * reduce PR size (bis) * reduce PR size (ter) * (quater) * convert self.shape to tuple for type stability * deprecate N, store _N_padded * call scattering_filter_factory with 2**self._N_padded * documentation * bugfix `N(self)` property * bugfix call to create_filters * update scattering_filter_factory prototype * update the numpy test * bugfix input length check test * clarify DeprecationWarning * bugfix * review by rastegah * remove unnecessary import * update DeprecationWarning * update documentation * rebase from PR 854

* FIX Update examples/1d/plot_filters.py As a consequence of #863. * FIX Update examples/2d/plot_filters Add new levels key.

lostanlen added 5 commits June 10, 2022 01:17

redefine self.N, deprecate self.J_pad

6fd6cf6

deprecationwarning

69acf11

restore self.vectorize (temporarily)

8aa55b7

bugfix _get_input_length

c8f87ae

J_pad = int(np.log2(self.N))

2221bba

MuawizChaudhary approved these changes Jun 9, 2022

View reviewed changes

Comment thread tests/scattering1d/test_utils_scattering1d.py

Merge branch 'dev' into redefine-N-1D-bis

1c59a9a

lostanlen changed the title ~~Redefine n 1 d bis~~ Redefine self.N in 1D, deprecate self.J_pad Jun 9, 2022

bugfix _check_input

f3c11bd

lostanlen marked this pull request as draft June 10, 2022 05:32

lostanlen added 8 commits June 10, 2022 07:33

revert to _check_input, write _get_shapes

6aa70c0

update _get_shapes

90bb0e9

try calling self._get_shapes in frontends

671732e

shape_fn in _get_shapes

5e5c198

write tensorflow shape_fn

7dd8178

move _check_input into _get_shapes

acd53cf

remove self._check_input

9b0bc9e

bugfix tensorflow shape check

19fd0b0

lostanlen changed the title ~~Redefine self.N in 1D, deprecate self.J_pad~~ Redefine self.N in 1D, deprecate self.J_pad, new 1D shape checks Jun 10, 2022

lostanlen marked this pull request as ready for review June 10, 2022 15:41

lostanlen added 3 commits June 11, 2022 07:55

reduce PR size

1fc2e64

_get_shapes was not a good idea

reduce PR size (bis)

8910cf9

reduce PR size (ter)

3ef38fd

lostanlen changed the title ~~Redefine self.N in 1D, deprecate self.J_pad, new 1D shape checks~~ Redefine self.N in 1D, deprecate self.J_pad Jun 11, 2022

(quater)

4dc679e

lostanlen requested a review from eickenberg June 12, 2022 05:17

convert self.shape to tuple for type stability

be99056

MuawizChaudhary requested changes Jun 15, 2022

View reviewed changes

Comment thread kymatio/scattering1d/filter_bank.py

Comment thread kymatio/scattering1d/frontend/numpy_frontend.py Outdated

remove unnecessary import

24b6206

MuawizChaudhary approved these changes Jun 15, 2022

View reviewed changes

update DeprecationWarning

c2bbc90

cyrusasfa approved these changes Jun 15, 2022

View reviewed changes

lostanlen mentioned this pull request Jun 15, 2022

Deprecate self.N in 2D #886

Merged

lostanlen added the 1D Issue with 1D scattering code label Jun 15, 2022

Merge branch 'dev' into redefine-N-1D-bis

282dab0

lostanlen added 3 commits June 17, 2022 16:52

update documentation

2470304

Merge branch 'redefine-N-1D-bis' of https://github.com/lostanlen/kymatio

dea23b1

into redefine-N-1D-bis

rebase from PR 854

6c1168c

cyrusasfa approved these changes Jun 18, 2022

View reviewed changes

MuawizChaudhary merged commit 32965c9 into kymatio:dev Jun 18, 2022

lostanlen mentioned this pull request Jun 18, 2022

self.M, self.N should be deprecated in 1D and 2D #881

Closed

janden reviewed Jun 20, 2022

View reviewed changes

janden mentioned this pull request Jun 20, 2022

Looks good but we're now missing docstring for N, J in scattering_filter_factory. #894

Closed

lostanlen mentioned this pull request Jun 20, 2022

plot_filters_1d has the wrong images #812

Closed

janden added a commit to janden/kymatio that referenced this pull request Jun 23, 2022

FIX Update examples/1d/plot_filters.py

45642e3

As a consequence of kymatio#863.

janden mentioned this pull request Jun 23, 2022

FIX Update examples/*d/plot_filters.py #899

Merged

lostanlen pushed a commit that referenced this pull request Jun 23, 2022

FIX Update examples/1d/plot_filters.py (#899)

cfbd1a3

* FIX Update examples/1d/plot_filters.py As a consequence of #863. * FIX Update examples/2d/plot_filters Add new levels key.

lostanlen deleted the redefine-N-1D-bis branch June 25, 2022 16:23

eickenberg pushed a commit that referenced this pull request Jul 5, 2022

FIX Update examples/1d/plot_filters.py (#899)

6e55d77

* FIX Update examples/1d/plot_filters.py As a consequence of #863. * FIX Update examples/2d/plot_filters Add new levels key.

Uh oh!

Conversation

lostanlen commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MuawizChaudhary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lostanlen commented Jun 9, 2022

Uh oh!

lostanlen commented Jun 11, 2022

Uh oh!

codecov-commenter commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

MuawizChaudhary left a comment

Choose a reason for hiding this comment

Uh oh!

lostanlen commented Jun 16, 2022

Uh oh!

cyrusasfa commented Jun 17, 2022

Uh oh!

lostanlen commented Jun 17, 2022

Uh oh!

lostanlen commented Jun 17, 2022

Uh oh!

MuawizChaudhary commented Jun 18, 2022

Uh oh!

janden left a comment

Choose a reason for hiding this comment

Uh oh!

janden commented Jun 20, 2022

Uh oh!

lostanlen commented Jun 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lostanlen commented Jun 9, 2022 •

edited

Loading

codecov-commenter commented Jun 11, 2022 •

edited

Loading