Skip to content

fix bug in FswavedecnResult.__setitem__ and improve docstrings#423

Merged
rgommers merged 5 commits into
PyWavelets:masterfrom
grlee77:fswavedecn_docs
Sep 15, 2018
Merged

fix bug in FswavedecnResult.__setitem__ and improve docstrings#423
rgommers merged 5 commits into
PyWavelets:masterfrom
grlee77:fswavedecn_docs

Conversation

@grlee77
Copy link
Copy Markdown
Contributor

@grlee77 grlee77 commented Sep 14, 2018

There was a bug in the dtype check in the setitem method of FswavedecnResult that prevents assigning modified coefficients. I think this probably warrants a 1.0.1 release to address.

The issue is that the user is supposed to be able to do something like:

dkey = (1, 2)
result[dkey] = modified_coeffs

A workaround in 1.0.0 is to do:

result.coeffs[result._get_coef_sl(dkey)] = modified_coeffs

This PR also improves testing of the FswaverecnResult and makes a few docstring improvements.

@grlee77 grlee77 added the bug label Sep 14, 2018
@grlee77
Copy link
Copy Markdown
Contributor Author

grlee77 commented Sep 14, 2018

It looks like numpy 1.9 did not support using assert_warns the way I had done in one of the new tests.

apparently use as a context manager requires numpy >= 1.11

this change is necessary for the tests to pass on numpy <1.11
@grlee77
Copy link
Copy Markdown
Contributor Author

grlee77 commented Sep 14, 2018

closes #424

@rgommers
Copy link
Copy Markdown
Member

quick question: the dict -> list change is doc-only (was a doc error), and then sl -> tuple(sl) change is for clarity? I.e. neither is a backwards compatibility break?

@grlee77
Copy link
Copy Markdown
Contributor Author

grlee77 commented Sep 15, 2018

I don't think there is a backcompat issue. The tuple(sl) is just to avoid numpy 1.15+ FutureWarnings that weren't caught previously. That method should be private, so I don't think it is an issue to change the return type there.

@rgommers rgommers merged commit 53f9cdb into PyWavelets:master Sep 15, 2018
@rgommers
Copy link
Copy Markdown
Member

Okay thanks @grlee77 . LGTM, merged.

@rgommers rgommers added this to the v1.0.1 milestone Sep 15, 2018
@rgommers
Copy link
Copy Markdown
Member

I think this probably warrants a 1.0.1 release to address.

From a few days from now till the end of October I have almost no bandwidth. Feel free to just push some commits through if you want to get a bug fix release out.

@grlee77
Copy link
Copy Markdown
Contributor Author

grlee77 commented Sep 20, 2018

Thanks. I will probably create 1.0.x branch starting from the tag for 1.0.0 and then cherry pick the commits from this PR and #427. I will then plan to make a 1.0.1 release in the next week or so.

grlee77 added a commit that referenced this pull request Sep 25, 2018
Backport bug fixes from master to 1.0.x (#423 and #427)
@grlee77 grlee77 deleted the fswavedecn_docs branch November 13, 2019 00:53
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.

2 participants