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

issues to address in moving towards 0.3.0 #61

Closed
grlee77 opened this issue Jul 13, 2015 · 13 comments
Closed

issues to address in moving towards 0.3.0 #61

grlee77 opened this issue Jul 13, 2015 · 13 comments
Milestone

Comments

@grlee77
Copy link
Contributor

grlee77 commented Jul 13, 2015

I opened this issue as a place for us to add/discuss issues to address for a future 0.3.0 release.

I took a quick look at a diff of current master and @nigma's pywt. It looks like the majority of the code should already be backwards compatible (and it already works with Python 3).

Here are a few issues I think need to be addressed:

  • A copy of six was included as _tools/six.py. It appears to only currently be used to import string_types here:
    https://github.com/rgommers/pywt/blob/496fab3ad1cc6fd35f359d560c9b0e4fa40da8a2/pywt/src/_pywt.pyx#L23
    Should we: 1.) keep this as a local copy, 2.) add six as a dependency, 3.) just define string_types directly ourselves?
  • current master already has one new function (idtwn) in multidim.py. Should this be kept in the initial v0.3.0 release because was already committed? We can change to the more efficient implementation I proposed in added wavedecn and waverecn to multilevel.py #52 for a later release.
  • In thresholding.py, the individual functions soft, hard, greater, less have been replaced with a single function called threshold that has a 'mode' argument to determine the threshold type.
    Should we provide wrappers with the old names for backwards compatiblity?
    i.e.
def soft(data, value, substitute=0):
    return threshold(data, value, mode='soft', substitute=substitute)

Backwards compatiblity would also require adding the following to __init__.py:

import tresholding

so that these would be called as pywt.thresholding.soft as in the old API

@kwohlfahrt
Copy link
Member

  • I would define string_types if it is necessary. On a quick check, it is only used to pre-screen inputs to MODES.from_object, and raises a TypeError if they are out of range or not stringy. MODES.from_object also checks whether they are in range, and getattr raises a TypeError anyway (python3 at least) if called with a non-string - unless Cython behaves differently I think this can be removed. I'll play with it in a couple of days.
  • No strong opinion on idwtn. I need it but I'll be using git master anyway so little difference.
  • Yes, I would define the old names (personally I would use functools.partial)

I would like to get #59 merged, just need to figure out how to define the CFLAGS for old gcc (gcc >= 5.1 defaults to -std=gnu11, which is sufficient).

@grlee77
Copy link
Contributor Author

grlee77 commented Jul 16, 2015

I agree on functools.partial.
I haven't had a chance to review #59 yet, but will try to get to it this week.

kwohlfahrt pushed a commit to kwohlfahrt/pywt that referenced this issue Jul 17, 2015
`_check_mode_input` was the only user of six. It was only used to check the
input of `MODES.from_object`, and raised a `TypeError` if a numeric mode was
out of range or a mode object was not a string type. `MODES.from_object` raises
a `ValueError` in these situations, and when the mode name string is not found.

Now, `MODES.from_object` is wrapped in a function `_try_mode` that returns its
result and fixes the type of any exception raised to keep the API the same.

This is a candidate for cleanup after the API break. Part of issue PyWavelets#61
@kwohlfahrt kwohlfahrt mentioned this issue Jul 17, 2015
@rgommers rgommers added this to the v0.3.0 milestone Jul 22, 2015
@rgommers
Copy link
Member

I'd keep idwtn. The v0.3.0 release should be fully backwards-compatible, but I don't see a reason not to add anything new. Removing it now and then re-adding it seems like effort that could be better spent elsewhere to me.

@rgommers
Copy link
Member

I've created v0.3.0 and v0.4.0 Milestones that we can tag issues and PRs with.

@rgommers
Copy link
Member

Agree on adding back the old names with functools.partial.

@rgommers
Copy link
Member

@kwohlfahrt is there a good reason to want to merge gh-59 right before instead of right after tagging v0.3.0? If not, I'd prefer after in order to reduce the risk (it's the kind of large PR that potentially can introduce a regression).

@kwohlfahrt
Copy link
Member

I have some other stuff brewing on top of it that I would have then put towards the next release, and given that #59 doesn't change any internal or external interfaces I thought it would be OK to merge for a backwards compatible release.

That was it, if you think it's too risky then we can postpone it too.

@kwohlfahrt
Copy link
Member

Also having had a closer look, I'm going to disagree with everybody (including my past self). Given that thresholding.threshold is essentially a switch and the different modes share no code, I think it would make more sense to restore the individual functions, and then simply call out to them from the combined one. Will open a PR tomorrow.

kwohlfahrt pushed a commit to kwohlfahrt/pywt that referenced this issue Jul 22, 2015
Each mode of the threshold function was independent, so they make more sense as
individual functions. This commit is just a code rearrangement, no change to
functionality.

Addresses PyWavelets#61
@rgommers rgommers mentioned this issue Jul 23, 2015
@rgommers
Copy link
Member

All issues discussed here are resolved, so how about tagging v0.3.0 tomorrow?

Regarding releases, I propose to just use the Github Releases feature (can add release notes and source code / binaries at https://github.com/rgommers/pywt/releases).

Furthermore we need something on PyPi. I'd like to just https://pypi.python.org/pypi/PyWavelets instead of a renamed fork (seems better for users), but that we need permission from @nigma. I'll open an issue about that on his repo.

@rgommers
Copy link
Member

Opened nigma/pywt#13 for PyPi access.

@rgommers
Copy link
Member

At least one more thing to do before tagging the release: update the metadata in setup.py (author, download url, supported Python versions, etc.).

@rgommers
Copy link
Member

This is all done, so closing.

@rgommers rgommers modified the milestone: v0.3.0 Jul 27, 2015
@rgommers
Copy link
Member

Tagged and sources on PyPi and https://github.com/PyWavelets/pywt/releases/tag/v0.3.0 now. Still want to produce some Windows wheels and put them on PyPi, after that send the release announcement.

aaren pushed a commit to aaren/pywt that referenced this issue Aug 3, 2015
`_check_mode_input` was the only user of six. It was only used to check the
input of `MODES.from_object`, and raised a `TypeError` if a numeric mode was
out of range or a mode object was not a string type. `MODES.from_object` raises
a `ValueError` in these situations, and when the mode name string is not found.

Now, `MODES.from_object` is wrapped in a function `_try_mode` that returns its
result and fixes the type of any exception raised to keep the API the same.

This is a candidate for cleanup after the API break. Part of issue PyWavelets#61
aaren pushed a commit to aaren/pywt that referenced this issue Aug 3, 2015
Each mode of the threshold function was independent, so they make more sense as
individual functions. This commit is just a code rearrangement, no change to
functionality.

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

No branches or pull requests

3 participants