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

added wavedecn and waverecn to multilevel.py #52

Closed
wants to merge 13 commits into from

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Jun 26, 2014

added an n-dimensional equivalent of wavedec2, waverec2

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6410d92 on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7ebdca7 on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 698b047 on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 698b047 on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4c7dd7f on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ea61135 on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 336e209 on grlee77:wavedecn into f7991ff on rgommers:master.

@rgommers
Copy link
Member

rgommers commented May 5, 2015

@grlee77 this repo has been quiet for a while, but I intend to pick up the pace here after scipy 0.16.0 is out the door.

Will take me a while to review this though. Looks good from a quick browse.

@rgommers
Copy link
Member

rgommers commented May 5, 2015

Also note that Github doesn't send notifications if you add commits, so best to leave a comment once you've pushed something that's ready for review/merging.

@grlee77
Copy link
Contributor Author

grlee77 commented May 5, 2015

Hi @rgommers,

I am not in any particular hurry to get it merged. I just needed a reasonably fast 3D wavedecn/waverecn for personal use and thought I would contribute it back to the project.

From testing on my machine on a 256x256 image, dwtn was taking approximately 70 ms prior to these changes and only about 1ms afterwards. This is comparable in performance to the Rice Wavelet Toolbox (https://github.com/ricedsp/rwt), but without the limitation to only 1D or 2D signals.

I also tried using a simple parallel/prange in the cython code, but for some reason this made things much slower again, so I commented it back out. Is there any policy regarding the use of OpenMP in the project?

One thing I will try to clean up a bit is that the previous versions of downcoef/upcoef are still present in _pywt.pyx, because they are still used a couple of places aside from the dtwn/idwtn functions I modified. I don't think it should be necessary to maintain both the old and new versions of these functions, though.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 4338266 on grlee77:wavedecn into f7991ff on rgommers:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 4338266 on grlee77:wavedecn into f7991ff on rgommers:master.

@rgommers
Copy link
Member

rgommers commented May 7, 2015

That speedup sounds very good!

There is such a policy - no OpenMP. I'd like to integrate this in Scipy, and there OpenMP isn't accepted (because it's not portable enough - see scipy/scipy#4374 (comment) for example).

remove extraneous print statement.
@rgommers rgommers added this to the v0.4.0 milestone Jul 22, 2015
@grlee77
Copy link
Contributor Author

grlee77 commented Jul 23, 2015

I recently rebased this pull request on my local machine to omit some changes made here that were unrelated to this pull request and to clean up the history, but now if I want to push the rebased code back up, it will require a fast foward. I also updated the commit messages to meet the numpy developer guidelines.

@rgommers / @kwohlfahrt :
I am still relatively new to git. What is the best way to proceed in this case? Should I just do the fast-forward since this has not been reviewed yet or is it better to close this pull request and just open a new one using the rebased commits?

Also there are really two separate things contributed here that might be better served as independent pull requests:
1.) adding wavedecn and waverecn python functions
2.) rewriting the dwtn, idwtn cython code for speed.
If I need to open a new pull request anyway, should I split those out to two separate pull requests?

@rgommers
Copy link
Member

In principle it's OK to just force-push your rebased branch to this PR. However you are right that in this case it would make sense to have separate PRs for (1) and (2). So now closing this and opening two new PRs seems like the way to go.

@grlee77
Copy link
Contributor Author

grlee77 commented Jul 24, 2015

OK. closing this one now and will open separate PRs going forward

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

Successfully merging this pull request may close these issues.

3 participants