jjhelmus / nmrglue

A module for working with NMR data in Python
BSD 3-Clause "New" or "Revised" License
211 stars 86 forks source link

Update proc_bl.py #40

Closed atomman closed 8 years ago

atomman commented 8 years ago

I used to use a baseline correction method from Anal. Chem., 2013, 85, 1231. I have implemented as described in the paper and I think i works quite well.

I have never added anything to a Git before so please advise if I'm doing it wrong.

There are a few subroutines which could be vectorized but I fail to see exactly how.

jjhelmus commented 8 years ago

Thanks for the contribution. This looks like a great addition to nmrglue! This addition needs a little bit of work before it can be merged.

First, currently the code does not work because the signal variable on line 296 is undefined. Adding import scipy.signal as signal to the imports at the top of the file fixes this.

Second, these new functions should be added to the documentation of the proc_bl module. A few lines will need to be added to the doc/source/reference/proc_bl.rst file.

Third, the style of this new code does not match that used in the rest of nmrglue. As much as possible nmrglue follows the PEP8 style guide which specifies among other things how variables and functions should be named (lowercase_with_underscores) and where and how much white-space should be included in code. It would probably be prudent to mark the helper function (rolling_window, GetSD, etc) as private by beginning the function names with an underscore. I find the pep8 tool helpful for checking code style

If you would like to address these concern that would be great. I'm also happy to clean up this code myself if you would prefer.

Finally, do not worry about the failing CI tests. There is a bug in the .travis.yml file which I need to fix.

atomman commented 8 years ago

I'll try and make the code PEP8 compliant. I'm not exactly clear on how and what goes into the proc_bl.rst file so some directions would be much appreciated.

jjhelmus commented 8 years ago

This is looking much better @atomman, just a few whitespace issue that should be addressed. Thanks for taking the time to clean it up.

As for the documentation, you will need to add the function name to the list in the User Function section in the doc/source/reference/proc_bl.rst file. You may want to give function a less generic name prior to this, maybe baseline_correct_wang or baseline_correct_dist_classification?

atomman commented 8 years ago

The reason i think "baseline_corrector" is an appropriate name is because the algoritmn is named so by the authors of the original paper.

jjhelmus commented 8 years ago

I'm fine with the baseline_corrector name is that is what is used in the paper. If you can cleanup the whitespace a bit and add the function to the documentation then I think this should be good to go.

jjhelmus commented 8 years ago

@atomman Can you add this new function to the docs? If you would prefer I can do this for you.

atomman commented 8 years ago

@jjhelmus I added the function name in the doc/source/reference/proc_bl.rst file, OK?

jjhelmus commented 8 years ago

Merged, thanks for all your work on this @atomman.