PyWavelets / pywt

PyWavelets - Wavelet Transforms in Python
http://pywavelets.readthedocs.org
MIT License
2.03k stars 469 forks source link

Keyword "per" for dwt extension mode #245

Closed pierrepaleo closed 7 years ago

pierrepaleo commented 7 years ago

When computing dwt/wavedec/packet, the keyword "per" should refer to the "periodization" mode. It would make a backward compatibility with the nigma/pywt version.

Example:

from scipy.misc import ascent
img = ascent() # (512, 512) image
P1 = pywt.WaveletPacket2D(img, "sym2", mode="periodization", maxlevel=1)
P1.decompose()
P2 = pywt.WaveletPacket2D(img, "sym2", mode="per", maxlevel=1)
P2.decompose()

print("P1 mode: %s, appcoeff 1 shape: %s" % (P1.mode, str(P1.a.data.shape))) # (256, 256) as expected
print("P2 mode: %s, appcoeff 1 shape: %s" % (P2.mode, str(P2.a.data.shape))) # (257, 257)

It looks like the computation is performed with the symmetric extension mode in the second case.

rgommers commented 7 years ago

Thanks for the bug report @pierrepaleo. Something looks to have gone pretty badly wrong here:

>>> from pywt._extensions._pywt import Modes
>>> Modes.from_object('per')
/home/rgommers/.local/bin/ipython:1: DeprecationWarning: per has been renamed to smooth and will be unavailable in a future version of pywt.
  #!/usr/bin/python
3
grlee77 commented 7 years ago

I believe I caused this when the new reflect edge mode was added. I didn't realize that the mode conversion implementation relies on the order of items in Modes.modes and _old_modes being the same.

I think there are two relatively easy solutions: 1.) move reflect to be the last element in Modes.modes list or 2.) make _old_modes a dict that translates old names to new

grlee77 commented 7 years ago

To further clarify, due to the order of reflect in the list, the following old modes are still converted correctly: 'zpd', 'cpd', 'sym'. The bug affects 'ppd', 'sp1', 'per'.

rgommers commented 7 years ago

That happens. I thought we had added a test covering this name mapping, but apparently not.

rgommers commented 7 years ago

@grlee77 this will affect quite a lot of users. Shall we do a 0.5.1 bugfix release soon?

grlee77 commented 7 years ago

@rgommers. a bug fix release sounds good. So far it would include #246 and #244. first, we should look into #247 as well, though.

rgommers commented 7 years ago

Great. I have a 0.5.x branch here: https://github.com/rgommers/pywt/tree/0.5.x

I propose to push that to the main repo and work off that.

grlee77 commented 7 years ago

@rgommers Sounds good. Is this the procedure you are suggesting?: 1.) check out your branch 2.) push it to a 0.5.x branch on PyWavelets/pywt 3.) make a PR to that branch to update setup.py/release notes 4.) create a v0.5.1 tag from that branch and then do the release as usual i.e. I can do tagging and release process from the branch in the same fashion as from master? (I am still relatively new to the release process)

rgommers commented 7 years ago

Indeed. Ive just done 1-2. For the rest it's the same as a normal release, just against this branch instead of master.

grlee77 commented 7 years ago

Thanks @rgommers. I did not get to it this week and will be traveling for a few days, but will try to get the release out by early next week.

rgommers commented 7 years ago

No worries, we're not in that much of a hurry I'd think.