Skip to content

MAINT decouple scattering_filter_factory from ScatteringBase1D.build - #842

Merged
MuawizChaudhary merged 5 commits into
kymatio:devfrom
lostanlen:fix-840
Jun 8, 2022
Merged

MuawizChaudhary merged 5 commits into
kymatio:devfrom
lostanlen:fix-840

Conversation

@lostanlen

@lostanlen lostanlen commented Jun 5, 2022 •

Copy link
Copy Markdown
Collaborator

Fixes #840
scattering_filter_factory should not return t_max_phi

  • Net reduction in LOC count
  • Simplification of the scattering1d/filter_bank module
  • Faster Scattering1D constructor
  • no more criterion_amplitude being passed to scattering_filter_factory
  • no more compute_minimum_support_to_pad
  • Backwards compatible as far as frontends are concerned

The calls to scattering_filter_factory will require a rebase from #833

@lostanlen lostanlen changed the title [WIP] decouple scattering_filter_factory from compute_minimum_support_to_pad [WIP] decouple scattering_filter_factory from ScatteringBase1D.build Jun 5, 2022
@codecov-commenter

codecov-commenter commented Jun 5, 2022 •

Copy link
Copy Markdown

Codecov Report

Merging #842 (0391dca) into dev (45afade) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #842      +/-   ##
==========================================
- Coverage   88.05%   87.93%   -0.12%     
==========================================
  Files          64       64              
  Lines        2311     2305       -6     
==========================================
- Hits         2035     2027       -8     
- Misses        276      278       +2     
Flag Coverage Δ
jenkins_main 87.93% <100.00%> (-0.12%) ⬇️

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

Impacted Files Coverage Δ
kymatio/scattering1d/filter_bank.py 98.76% <100.00%> (-1.24%) ⬇️
kymatio/scattering1d/frontend/base_frontend.py 98.61% <100.00%> (ø)
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...0391dca. Read the comment docs.

@MuawizChaudhary MuawizChaudhary changed the title [WIP] decouple scattering_filter_factory from ScatteringBase1D.build MAINT decouple scattering_filter_factory from ScatteringBase1D.build Jun 5, 2022
@lostanlen
lostanlen requested a review from MuawizChaudhary June 5, 2022 23:11
lostanlen added 3 commits June 6, 2022 01:44
* remove compute_minimum_support_to_pad
* bugfix previous commit
(E       IndexError: tuple index out of range) due to bad phi_f shape
* update examples/1d/plot_filters
* remove criterion_amplitude from self
remove fourth output argument (t_max_phi)
@lostanlen lostanlen added the 1D Issue with 1D scattering code label Jun 6, 2022
@lostanlen
lostanlen requested a review from changhongw June 7, 2022 14:23
@MuawizChaudhary
MuawizChaudhary requested a review from cyrusasfa June 7, 2022 14:28

@MuawizChaudhary MuawizChaudhary left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tests pass on my end.

Comment thread examples/1d/plot_filters.py
Comment thread kymatio/scattering1d/frontend/base_frontend.py
Comment thread tests/scattering1d/test_numpy_scattering1d.py

@MuawizChaudhary MuawizChaudhary left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry, didn't mean to approve.

requested by muawiz
@lostanlen
lostanlen requested a review from MuawizChaudhary June 8, 2022 16:01
@MuawizChaudhary
MuawizChaudhary merged commit e0978a9 into kymatio:dev Jun 8, 2022
@lostanlen
lostanlen deleted the fix-840 branch June 25, 2022 17:12
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
…ld` (#842)

* remove call to compute_minimum_support_to_pad

* remove criterion_amplitude from scattering_filter_factory

* remove compute_minimum_support_to_pad
* bugfix previous commit
(E       IndexError: tuple index out of range) due to bad phi_f shape
* update examples/1d/plot_filters
* remove criterion_amplitude from self

* update test_Scattering1D_filter_factory_T

remove fourth output argument (t_max_phi)

* remove 4th output argument from scattering_filter_factory

* remove an empty line

requested by muawiz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1D Issue with 1D scattering code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants