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

mdwt,midwt enhancements #13

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

mborgerding
Copy link

@mborgerding mborgerding commented Feb 7, 2017

mdwt and midwt now handle

  • complex data, vs just real
  • single precision data, vs just double
  • multidimensional arrays, transforms are done on each leading vector or matrix (as in fft2)
  • optional new arguments to choose 1d or 2d transforms (default is same as previous behavior)

Tests were added for new modes.

This branch closes issue #12

@kalliste
Copy link
Contributor

kalliste commented Feb 7, 2017

This is great.

Rice has asked me to get a contributor agreement from everyone for this project - can we get you to sign one?

https://mega.nz/#!sEJ2AIaa!hMBS2sXCgrH2UWq7oX8_VhY-1O3UHZW1cM-r3Ep8N8U

@mborgerding
Copy link
Author

I sent the CLA to your email a week ago. What's the status?

@kalliste
Copy link
Contributor

Your contribution looks great. Before merging it I'll still want to personally verify:

  • No regressions on any of MATLAB, Octave, or Python
  • No major performance loss for using C++
  • EE students are just as happy reading C++ as C
  • Outside confirmation that this is the correct way to handle complex values

Also will want to deal with:

  • Similar implementations for rdwt and irdwt
  • A minor concern but still might care.. consistency in indent style - the rest of the code uses 1TBS

Currently no one is being paid to work on wavelet toolbox so I am guessing that this will have to wait until I clear out my queue of paying work.

@mborgerding
Copy link
Author

bump

@kalliste
Copy link
Contributor

kalliste commented Jul 9, 2019

I ought to have stated clearly what would be required to get this merged before asking someone to go to the trouble of signing a contributor agreement. Sorry about that - that's on me to be a better maintainer.

I won't merge this change for mdwt and midwt without matching changes for rdwt and irdwt and also the code style should be kept consistent across the whole package.

After watching a video of Howard Hinnant looking at the assembly output of his C++ date and time code vs C I have concluded that I am ok with the language switch. Also the hypothetical Rice EE undergraduate students can get some tough love and learn to read C++ template code.

That just leaves regression testing and getting someone to convince me that this is the correct way to handle complex values. If someone cares enough to handle the rdwt and irdwt implementations and consistent code formatting then I will plan to match their effort by doing the regression testing and reaching out to a professor or someone about complex values.

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

Successfully merging this pull request may close these issues.

2 participants