Skip to content

MAINT Simplify filter_bank.py #833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jun 9, 2022
Merged

MAINT Simplify filter_bank.py #833

merged 5 commits into from
Jun 9, 2022

Conversation

lostanlen
Copy link
Collaborator

@lostanlen lostanlen commented Jun 3, 2022 •
edited
Loading

closes #825
fewer lines, shorter docs, faster tests

  • remove P_max=5, eps=1e-7, and normalize="l1" from Scattering1D object
  • this lightens the kwarg lists of scattering_filter_factory and calibrate_scattering_filters
  • unify morlet_1d and gauss_1d algorithms
  • delete get_normalizing factors

supersedes #831 and #832
many thanx to @MuawizChaudhary

@lostanlen lostanlen requested a review from MuawizChaudhary June 3, 2022 23:07
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022 •
edited
Loading

Codecov Report

Merging #833 (370f4d9) into dev (45afade) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #833      +/-   ##
==========================================
- Coverage   88.05%   87.86%   -0.20%     
==========================================
  Files          64       64              
  Lines        2311     2282      -29     
==========================================
- Hits         2035     2005      -30     
- Misses        276      277       +1
Flag Coverage Δ
jenkins_main 87.86% <100.00%> (-0.20%) ⬇️

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

Impacted Files Coverage Δ
kymatio/scattering1d/frontend/base_frontend.py 98.55% <ø> (-0.07%) ⬇️
kymatio/scattering1d/filter_bank.py 99.27% <100.00%> (-0.73%) ⬇️
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 45afade...370f4d9. Read the comment docs.

@lostanlen lostanlen added the 1D Issue with 1D scattering code label Jun 3, 2022
@MuawizChaudhary MuawizChaudhary changed the title Simplify filter_bank.py MAINT Simplify filter_bank.py Jun 4, 2022
This was referenced Jun 5, 2022
Closed
Merged
@lostanlen
simplify filter_bank.py
370f4d9 
closes kymatio#825
* remove P_max=5, eps=1e-7, and normalize="l1" from Scattering1D object
* this lightens the kwarg lists of scattering_filter_factory, calibrate_scattering_filters,
* unify morlet_1d and gauss_1d algorithms
* delete get_normalizing factors
@lostanlen lostanlen mentioned this pull request Jun 6, 2022
Closed
@lostanlen
Copy link
Collaborator Author

lostanlen commented Jun 6, 2022 •
edited
Loading

My not-so-secret plan after this:

  • remove adaptive_choice_P, make it part of morlet_1d (with appropriate comments to explain the formula)
  • move periodize_filter_fourier to scattering1d/utils.py and remove its default value n_periods=1
  • remove compute_sigma_psi, make it part of compute_params_filterbank (with appropriate comments)
  • move compute_xi_max to scattering1d/utils.py
  • remove calibrate_scattering_filters, make it part of scattering_filter_factory (i.e., scattering_filter_factory calls compute_params_filterbank directly)

Once that is done i think we'll be able to say that issue #513 is solved ;)
All that will remain in filter_bank.py will be:

  • morlet_1d
  • gauss_1d (which itself calls morlet_1d with xi=None)
  • get_max_dyadic_subsampling (called twice by compute_params_filterbank)
  • compute_params_filterbank (called twice by scattering_filter_factory)
  • scattering_filter_factory

This will open an avenue towards modularizing filterbank construction, by switching to other wavelets (Morse/Gammatones/Meyer/other) and other compute_params_filterbank functions (the one we have, introduced by @janden, has a linear portion and an exponential portion, but in other cases we might want to guarantee that the xi's follow a geometric progression and have the sigma's be an affine function of xi's, as in ERB/"VQT" auditory filterbanks).

MuawizChaudhary
MuawizChaudhary approved these changes Jun 6, 2022
View reviewed changes
Copy link
Collaborator

@MuawizChaudhary MuawizChaudhary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@lostanlen lostanlen mentioned this pull request Jun 7, 2022
Closed
@lostanlen lostanlen changed the title MAINT Simplify filter_bank.py MAINT Simplify filter_bank.py Jun 7, 2022
@lostanlen lostanlen requested a review from changhongw June 7, 2022 14:25
@lostanlen
Copy link
Collaborator Author

Rebased

MuawizChaudhary
MuawizChaudhary approved these changes Jun 9, 2022
View reviewed changes
Copy link
Collaborator

@MuawizChaudhary MuawizChaudhary left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

EDIT: Actually, the tests are failing. PLS fix

@lostanlen
Copy link
Collaborator Author

Fixed. That was my mistake in rebase

@MuawizChaudhary MuawizChaudhary merged commit 132dcf9 into kymatio:dev Jun 9, 2022
@MuawizChaudhary MuawizChaudhary mentioned this pull request Jun 14, 2022
Closed
@lostanlen lostanlen deleted the fix-825-bis branch June 25, 2022 17:11
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
@lostanlen @eickenberg
MAINT Simplify filter_bank.py (#833)
b719579 
* simplify filter_bank.py

closes #825
* remove P_max=5, eps=1e-7, and normalize="l1" from Scattering1D object
* this lightens the kwarg lists of scattering_filter_factory, calibrate_scattering_filters,
* unify morlet_1d and gauss_1d algorithms
* delete get_normalizing factors

* update gauss_1d call to have min_to_pad

* finish rebasing. tests pass locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MuawizChaudhary MuawizChaudhary MuawizChaudhary approved these changes

@changhongw changhongw Awaiting requested review from changhongw

@cyrusvahidi cyrusvahidi Awaiting requested review from cyrusvahidi

@janden janden Awaiting requested review from janden

Assignees
No one assigned
Labels
1D Issue with 1D scattering code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lostanlen @codecov-commenter @MuawizChaudhary