Automatic phase correction - Githubissues

jjhelmus / nmrglue

A module for working with NMR data in Python

BSD 3-Clause "New" or "Revised" License

211 stars 86 forks source link

Automatic phase correction #31

Closed mfitzp closed 9 years ago

mfitzp commented 9 years ago

An initial implementation of automatic phase correction for consideration. This adds a new function to proc_base of autops, alongside two example phase optimisation (scoring) algorithms: ACME and a naive peak minima approach.

Standard algorithms can be selected by name ('acme', 'peak_minima') while custom algorithms can be provided as a function.

A demo notebook is available.

mfitzp commented 9 years ago

I have implemented this in proc_base because that is where the ps function is. However, it may make sense to move this into a separate module (as with the baseline, etc.) for clarity. The existing ps functions could be imported back into proc_base for compatibility with existing code.

Suggestions for improvements, additional algorithms to implement, etc. are most welcome.

jjhelmus commented 9 years ago

@mfitzp This is a great addition to nmrglue, thanks for making it available. A few comments:

Could these functions be move into a new file in the process directory, maybe proc_autophase.py? I don't consider automatic phase to be a "basic" processing technique so I don't think it belongs in proc_base.py
The ps_acme and ps_peak_minima function could have better names to reflect that they are calculating a score I think making them private would be prudent, _ps_acme_score and ps_peak_minima_score sound good to be.
A few lines have some style issues. nmrglue tried to follow PEP8 as much as possible. Specifically lines 999 and 1000 are too long (79 character max), a few lines have extraneous whitespace and there is an extra blank line before the autops function.
Do these functions work on two dimensional data? If not this should be mentioned in the docstring.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling 3dc8d867d83a3507ae73e460c73ff4e7907ac30b on mfitzp:autophasecorrect into 8974b1357c5de93c82c6362e19ce26f5a0a2b69d on jjhelmus:master.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling f5a9833bc704e55602654ff198e5d7a4a0b49b15 on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling d602e0c1f5f447d46604be556ef4ca1f74929380 on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling d602e0c1f5f447d46604be556ef4ca1f74929380 on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling 5ea59bcccac541de61f3d8afbb1902e23cf0784a on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling 5ea59bcccac541de61f3d8afbb1902e23cf0784a on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

mfitzp commented 9 years ago

@jjhelmus Thanks for the feedback (and apologies for the coveralls update spam; I got caught rebasing down to a single commit). In the update to the PR I've moved the autophase code into proc_autophase as autops and renamed the score functions as suggested _ps_acme_score etc.

I've fixed the PEP8 issues, though there is some white-space left in calculations for clarity.

At present the algorithms are only tested for 1D spectra but should be possible to adapt for 2D with some thought. Unfortunately I don't have any unphased 2D data (or any, actually) to test with. If you're able to provide some I'll see what I can do!

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling 5ea59bcccac541de61f3d8afbb1902e23cf0784a on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

coveralls commented 9 years ago

Coverage decreased (-0.02%) to 24.22% when pulling 5ea59bcccac541de61f3d8afbb1902e23cf0784a on mfitzp:autophasecorrect into d31807b80d416daedfaeb1ecec053e7b7381d137 on jjhelmus:master.

jjhelmus commented 9 years ago

@mfitzp There are a few examples of 2D data at the nmrglue Google code download page that could be used to develop a 2D version of the autophasing algorithm. In particular the data from the process_pipe_2d example might be a good choice.

I merged in this branch as is, made some minor style changes and added the module to the documentation. Feel free to open a new PR for the 2D work. Thanks again for the contribution.