Skip to content
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

idwtn should allow coefficients to be set as None #291

Merged
merged 4 commits into from
Mar 9, 2017

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Mar 2, 2017

The current behavior in idwtn does not match the description in the docstring when any of the values in the coeffs dictionary is None. This PR updates the behavior to match the docstring. This also makes idwtn treat None coefficients in the same was as idwt2.

Previously idwtn raised an error if any coefficients were None.

technically that is a change in behavior, but as it is a bug fix I don't think we should need a deprecation cycle?

closes gh-290

@grlee77 grlee77 added the bug label Mar 2, 2017
@grlee77 grlee77 added this to the v1.0 milestone Mar 2, 2017
@grlee77 grlee77 mentioned this pull request Mar 2, 2017
@codecov-io
Copy link

codecov-io commented Mar 2, 2017

Codecov Report

Merging #291 into master will decrease coverage by -0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   86.36%   86.26%   -0.11%     
==========================================
  Files          21       21              
  Lines        3088     3086       -2     
  Branches      532      532              
==========================================
- Hits         2667     2662       -5     
- Misses        368      371       +3     
  Partials       53       53
Impacted Files Coverage Δ
pywt/_multidim.py 97.77% <100%> (ø)
pywt/_extensions/_swt.pyx 75.96% <0%> (-3.29%)

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 492c434...0367caf. Read the comment docs.

@rgommers
Copy link
Member

rgommers commented Mar 7, 2017

technically that is a change in behavior, but as it is a bug fix I don't think we should need a deprecation cycle?

I agree

@rgommers
Copy link
Member

rgommers commented Mar 7, 2017

LGTM for idwtn, however as far as I can tell the idwt2 behavior is not documented or tested. Maybe good to add that?

@grlee77
Copy link
Contributor Author

grlee77 commented Mar 7, 2017

Added an idwt2 test and updated the docstrings to describe the expected behavior.

@rgommers rgommers merged commit 4fa4b45 into PyWavelets:master Mar 9, 2017
@rgommers
Copy link
Member

rgommers commented Mar 9, 2017

Merged, thanks @grlee77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idwtn should treat coefficients set to None as zeros
3 participants