Skip to content

Removed redundant default value for alpha #814

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.

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

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

MarcusHaggbom
Copy link
Contributor

Backwards compatible
No new functionality
Note! Test function was modified to use a value of alpha=5.

Removed default value for alpha parameter everywhere in filter_bank.py except factory function (scattering_filter_factory). This way the value of alpha is set once, avoiding a possible override of this value by the default.

@lostanlen
Copy link
Collaborator

@janden how can i restart this CI so that we retry the build on Jenkins?
and how do i run "self-hosted CI" ?

@janden janden force-pushed the filterfactory1d_alpha branch from d45ac7e to d6f7816 Compare May 27, 2022 08:37
@janden
Copy link
Collaborator

janden commented May 27, 2022

I have rebased this on top of the new dev, so the CI should be working again.

@janden
Copy link
Collaborator

janden commented May 27, 2022

@MarcusHaggbom Remind me again what alpha does and why it's redundant?

@codecov-commenter
Copy link

Codecov Report

Merging #814 (d6f7816) into dev (ca9073f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev     #814   +/-   ##
=======================================
  Coverage   88.03%   88.03%           
=======================================
  Files          64       64           
  Lines        2323     2323           
=======================================
  Hits         2045     2045           
  Misses        278      278           
Flag Coverage Δ
jenkins_main 88.03% <100.00%> (ø)

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

Impacted Files Coverage Δ
kymatio/scattering1d/utils.py 100.00% <ø> (ø)
kymatio/scattering1d/filter_bank.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 ca9073f...d6f7816. Read the comment docs.

@MarcusHaggbom
Copy link
Contributor Author

@MarcusHaggbom Remind me again what alpha does and why it's redundant?

Alpha works as a parameter on how much aliasing error is tolerated when subsampling, and hence affects the number of "allowed" dyadic subsamplings for a wavelet.
The redundancy comes from assigning a default value to alpha in multiple places; when querying for filters given a certain value for alpha, this value should be used throughout the module. By requiring it to be passed explicitly in later functions, we mitigate the risk of (in future versions) forgetting to pass the intended value, which otherwise could cause a silent error. I.e.:

def main(alpha=1):
    _sub1(alpha)

def _sub1(alpha):
    _sub2()  # Forgetting to pass alpha

def _sub2(alpha=1):
    print(alpha)

main(0)

@lostanlen
Copy link
Collaborator

ready to merge

@lostanlen
Copy link
Collaborator

@MuawizChaudhary merge ?

@MuawizChaudhary MuawizChaudhary merged commit df87489 into kymatio:dev Jun 2, 2022
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
Co-authored-by: Marcus <haggbo@kth.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants