Update proc_autophase.py #48

atomman · 2016-03-04T13:13:17Z

The Automatic phase correction did not work as intended. Especially the penalty function was calculated incorrectly. I think.
Also there should be an option to set the derivative order and a penalty function scaling factor, gamma.

I would suggest that the function also returned the optimum phases.
In the case where 100's of spectra should be processed it would be nice to be able to store the phases in case one wants to reprocess.

The Automatic phase correction did not work as intended. Especially the penalty function was calculated incorrectly. I think. Also there should be an option to set the derivative order and a penalty function scaling factor, gamma.

Needed to take the absolute of the n-derivative of the spectrum

kfritzsc · 2016-03-04T15:00:10Z

nmrglue/process/proc_autophase.py

@@ -68,35 +68,42 @@ def _ps_acme_score(ph, data):
 Value of the objective function (phase score)

 """
- stepsize = 1
-
+ """


I think this is why the Travis CI tests are failing.

Edited: there are two many parentheses for the doctoring. One of the parenthesis is not indented correctly.

I have made changes to the autops function to accept the argument for the gamma constant. Also I have set the options to return the phases in a tuple. I'm unsure about how to describe these changes in the doc?

jjhelmus · 2016-03-07T17:53:26Z

This update seems to have broken the 'peak_minima' algorithm and neither algorithm is giving good results for some toy data I tried. @atomman can you provide an example of where this autops function works well?

atomman · 2016-03-09T19:10:48Z

OK. I see the problem. On the master of my branch(examples/processing/ACME test.ipynb) I have uploaded a notebook showing the new implementation compared to the current one. In order access the acme and peak_minima through autops one must pass the new gamma variable as a dummy variable. I dont like this hack and suggest that we create separate functions for acme and peak_minima auto phase functionality.

PS. Should we output a reference to the acme paper when the algorithm is used to remind the user of the citation? Or is it OK just to have it in the docs?

jjhelmus · 2016-03-09T21:06:32Z

PS. Should we output a reference to the acme paper when the algorithm is used to remind the user of the citation? Or is it OK just to have it in the docs?

I think keeping it it the docs only is acceptable.

Replaced autops with functions psacme and pspmin. As the two methods relies on different input the should have their own functions I think.

atomman · 2016-03-12T19:12:41Z

Why did the CI test fail?

On aside note: What is the recommended strategy for testing a branch with modified code?
If I download and install, it overwrites my current nmrglue. How to rename the package so it installs as say "tmpnmrglue"?

jjhelmus · 2016-03-15T20:19:23Z

nmrglue/process/proc_autophase.py

@@ -14,46 +14,49 @@
 from .proc_base import ps


-def autops(data, fn, p0=0.0, p1=0.0):
+def psacme(autops(data, p0=0.0, p1=0.0, gamma=5.0e-3, optreturn=False):


This syntax is not allowed.

jjhelmus · 2016-03-15T20:29:53Z

@atomman I marked the lines which are causing the CI to fail.

There are a number of methods for setting up a development environment which allows the modified version of nmrglue to be used as the default version. The method I uses is as follows:

Remove any previous installations of nmrglue.
Fork the nmrglue repository on GitHub using the top right "Fork" button".
In a development directory use git clone address_of_your_fork to clone your fork of nmrglue.

The source code in the new directory can be imported directly if the directory is added to the Python search path. I accomplish by adding a ".pth" file to the appropriate site-packages directory on my system. This file is a text file containing the path to the directory containing the nmrglue source . Another options is to add the source directory to the PYTHONPATH environmental variable.

chatcannon · 2016-03-16T17:37:25Z

To avoid overwriting your current install, you can install in a virtualenv:

virtualenv nmrglue-dev
source nmrglue-dev/bin/activate
pip install numpy scipy
pip install --editable [path-to-nmrglue-git-checkout]

coveralls · 2016-09-16T13:43:57Z

Changes Unknown when pulling bf24986 on atomman:patch-3 into * on jjhelmus:master*.

jjhelmus · 2016-09-16T20:01:55Z

nmrglue/process/proc_autophase.py

@@ -14,46 +14,46 @@
 from .proc_base import ps


-def autops(data, fn, p0=0.0, p1=0.0):
+def psacme(data, p0=0.0, p1=0.0, gamma=5.0e-3):


I do not think the name of this function should be changed. If it is the name in the documentation must also be changed.

jjhelmus · 2016-09-16T20:03:47Z

@mfitzp Can change you could take a look at these changes as you were to creator of the proc_autophase module. I do not know enough about the algorithms used to provide a good review of if these changes are reasonable.

mfitzp · 2016-09-16T21:14:11Z

@jjhelmus Sure, will take a look over the weekend.

@atomman the changes are difficult to assess as you've 'broken' the existing interface. The way the current algorithms were set up was to provide all the algorithms through a single function 'autops', with the algorithm selectable via fn. By removing this you remove this possibility.

Can you refactor based on the original interface?

If there are changes you would like to make to the API they would probably be better submitted as another PR (although I of course defer to @jjhelmus on this)

atomman · 2016-09-17T05:41:14Z

@mfitzp i will try and make the changes over the weekend. The reason I
suggested the change in the first place was to be able to provide a user
defined gamma. This variable is only relevant for acme and I could not come
up with an good common interface for multiple autophase functions relying
on different inputs. Any suggestions?

On Fri, 16 Sep 2016, 23:14 Martin Fitzpatrick, [email protected]
wrote:

@jjhelmus https:/jjhelmus Sure, will take a look over the
weekend.

@atomman https:/atomman the changes are difficult to assess
as you've 'broken' the existing interface. The way the current algorithms
were set up was to provide all the algorithm through a single function
'autops', with the algorithm selectable via fn. By removing this you
remove this possibility.

Can you refactor based on the original interface?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#48 (comment), or mute
the thread
https:/notifications/unsubscribe-auth/AJwFa6PgXDkhBBAT3rMfm8CMhRoDkrGAks5qqwakgaJpZM4HpWkc
.

Sent from my phone
/Kresten

mfitzp · 2016-09-17T10:12:07Z

@atomman How about setting autops to take variable arguments and pass these into the acme function? e.g.

def autops(data, fn, p0=0.0, p1=0.0, *args):
    ...
    opt = scipy.optimize.fmin(fn, x0=opt, args=(data, )+args)

....
def _ps_acme_score(ph, data, gamma=5.0e-3):

This way any function can ave extra parameters sent to it (unfortunately fmin doesn't take keyword args which would be nicer).

What do you think?

The reason for the current API was to allow custom functions to be used easily. But there is probably an argument for method-specific functions (e.g. acmeps like you've done here) so they can be documented individually. But it would be nice to keep the general interface too.

@mfitzp

@mfitzp, I changed the functions back so they are compatible with the 'autops' function. Now 'autops' accepts keywords(gamma and order) via '**kwargs' which are passed to '_ps_acme_score'. They are also passed to ' _ps_peak_minima_score' but are not used. This is the best solution I have found.

coveralls · 2016-09-20T06:47:52Z

Changes Unknown when pulling 2e2ae0e on atomman:patch-3 into * on jjhelmus:master*.

atomman · 2016-09-20T06:52:45Z

@mfitzp, I changed the functions back so they are compatible with theautops function. Now autops accepts keywords(gamma and order) via **kwargs which are passed to _ps_acme_score. They are also passed to _ps_peak_minima_score but are not used.
This is the best solution I have found.

mfitzp · 2016-09-20T06:56:35Z

@atomman Thanks, but in the latest commit there is no autops function at all! :)

You shouldn't need to accept the arguments as kwargs in the _ps_acme_score function (they also aren't actually kwargs, but args! ...and to accept them into a dict you would need to use **kwargs). If the call from autops is structured as shown below:

opt = scipy.optimize.fmin(fn, x0=opt, args=(data, )+args)

...the individual arguments will be passed to _ps_peak_minima_score as normal function arguments. You should be able to pick them up as:

def _ps_acme_score(ph, data, gamma=5.0e-3, order=<something>):

The gamma, etc. above will accept the arguments as long as they are coming in the correct order. To accept and discard additional arguments you can add *args to the end, e.g. for _ps_peak_minima_score:

def _ps_peak_minima_score(ph, data, *args):

Sorry for the faff. If this isn't making any sense I can put together a complete example.

forgot autops function in last commit

coveralls · 2016-09-20T07:32:31Z

Changes Unknown when pulling e61f2ad on atomman:patch-3 into * on jjhelmus:master*.

atomman · 2016-09-20T07:36:37Z

@mfitzp Thank you for the tips.
I think the current solution is quite nice. It would be nice if _ps_acme_score would default to an appropriate gamma and order parameter if they are not set by the user.

Added a few white spaces and changed a np.sum to np.nansum

coveralls · 2016-09-20T11:11:19Z

Changes Unknown when pulling 6f49c45 on atomman:patch-3 into * on jjhelmus:master*.

jjhelmus · 2016-09-23T18:12:14Z

nmrglue/process/proc_autophase.py


- return phasedspc
+ phasedspc = ng.proc_base.ps(data, p0=opt[0], p1=opt[1])


ng.proc_base.ps is not valid here. This should be ps

jjhelmus · 2016-09-23T18:12:28Z

nmrglue/process/proc_autophase.py


-def _ps_acme_score(ph, data):
+def _ps_acme_score(ph, data, kwargs):


jjhelmus · 2016-09-23T18:12:51Z

nmrglue/process/proc_autophase.py

 phc0, phc1 = ph
+ s = ng.proc_base.ps(data, p0=phc0, p1=phc1)


I will fix this

jjhelmus · 2016-09-23T18:13:37Z

nmrglue/process/proc_autophase.py



-def _ps_peak_minima_score(ph, data):
+def _ps_peak_minima_score(ph, data, kwargs):


@jjhelmus I'm a little unsure how to do this. The main functions 'autops' accepts **kwargs. Then these are passed to _ps_acme_score as kwargs. The _ps_acme_score needs additional parameters; here implemented as kwargs['gamma'] and kwargs['order']

jjhelmus · 2016-09-23T18:13:48Z

nmrglue/process/proc_autophase.py

@@ -122,7 +123,7 @@ def _ps_peak_minima_score(ph, data):

 phc0, phc1 = ph

- s0 = ps(data, p0=phc0, p1=phc1)
+ s0 = ng.proc_base.ps(data, p0=phc0, p1=phc1)


coveralls · 2016-09-23T18:18:53Z

Changes Unknown when pulling ec4e921 on atomman:patch-3 into * on jjhelmus:master*.

jjhelmus · 2016-10-05T16:36:57Z

@atomman The changes made to the ACME algorithm seem to have broken the process. I'm using the following script as a test case:

import numpy as np
import matplotlib.pyplot as plt
import nmrglue as ng
from nmrglue.process.proc_autophase import autops

def complex_lorentzian(x, x_0, width, amp):
    return amp * (1. / (width + 1.j * (x-x_0)))

# simulate a spectrum with 2 lorentzians and noise
x = np.arange(-512, 512, dtype='complex64')
peak1 = complex_lorentzian(x, -200, 10, 1000)
peak2 = complex_lorentzian(x, 100, 15, 1500)
np.random.seed(0)
noise = np.random.uniform(size=(1024, ))
spectrum = peak1 + peak2 + noise

# apply linear phase to the spectrum to create 'raw' un-phased spectrum
p0 = 48.
p1 = 3.
raw = ng.proc_base.ps(spectrum, p0=p0, p1=p1, inv=True)

# phase raw spectrum using various techniques
manual = ng.proc_base.ps(raw, p0=p0, p1=p1)
_, acme = autops(raw, 'acme', order=1, gamma=0)
_, peak_min = autops(raw, 'peak_minima', order=2, gamma=2)

# plot spectrums
p1 = plt.plot(raw.real, 'r-')
p2 = plt.plot(manual.real, 'k-')
p3 = plt.plot(acme.real, 'b-')
p4 = plt.plot(peak_min.real, 'g-')
plt.legend(['Raw', 'Manual', 'ACME', 'Peak Minima'])
plt.show()

With these changes I am get the following results. Notice that that the blue line phased using the ACME method is not correct.

Without these changes I get a reasonable phasing of the spectrum using the ACME method:

Also, please correct the PEP8 style issues in the new code and add reasonable defaults for the order and gamma parameters.

atomman · 2016-10-05T17:11:27Z

Dear @jjhelmus. I will make the suggested corrections. The reason it does
not work in your example is that gamma is set to zero. Try setting gamma to
a value different from zero, ex. 5e-3, otherwise negative amplitudes in the
spectrum will not be penalized and the correct phase will not be found.

On Wed, 5 Oct 2016, 18:37 Jonathan J. Helmus, [email protected]
wrote:

@atomman https:/atomman The changes made to the ACME
algorithm seem to have broken the process. I'm using the following script
as a test case:

import numpy as npimport matplotlib.pyplot as pltimport nmrglue as ngfrom nmrglue.process.proc_autophase import autops
def complex_lorentzian(x, x_0, width, amp):
return amp * (1. / (width + 1.j * (x-x_0)))

simulate a spectrum with 2 lorentzians and noise

x = np.arange(-512, 512, dtype='complex64')
peak1 = complex_lorentzian(x, -200, 10, 1000)
peak2 = complex_lorentzian(x, 100, 15, 1500)
np.random.seed(0)
noise = np.random.uniform(size=(1024, ))
spectrum = peak1 + peak2 + noise

apply linear phase to the spectrum to create 'raw' un-phased spectrum

p0 = 48.
p1 = 3.
raw = ng.proc_base.ps(spectrum, p0=p0, p1=p1, inv=True)

phase raw spectrum using various techniques

manual = ng.proc_base.ps(raw, p0=p0, p1=p1)
_, acme = autops(raw, 'acme', order=1, gamma=0)
_, peak_min = autops(raw, 'peak_minima', order=2, gamma=2)

plot spectrums

p1 = plt.plot(raw.real, 'r-')
p2 = plt.plot(manual.real, 'k-')
p3 = plt.plot(acme.real, 'b-')
p4 = plt.plot(peak_min.real, 'g-')
plt.legend(['Raw', 'Manual', 'ACME', 'Peak Minima'])
plt.show()

With these changes I am get the following results. Notice that that the
blue line phased using the ACME method is not correct.

[image: figure_1-1]
https://cloud.githubusercontent.com/assets/1050278/19122274/56a10e74-8aef-11e6-86ee-bbc350d1650b.png

Without these changes I get a reasonable phasing of the spectrum using the
ACME method:

[image: figure_1-2]
https://cloud.githubusercontent.com/assets/1050278/19122352/b335d66a-8aef-11e6-8246-7cf3e4436af8.png

Also, please correct the PEP8 https://www.python.org/dev/peps/pep-0008/
style issues in the new code and add reasonable defaults for the order
and gamma parameters.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#48 (comment), or mute
the thread
https:/notifications/unsubscribe-auth/AJwFa952kCtvRkC6QsiUajddsy-8pGUuks5qw9IqgaJpZM4HpWkc
.

Sent from my phone
/Kresten

jjhelmus · 2016-10-05T20:09:57Z

nmrglue/process/proc_autophase.py

- p = 1000 * pfun
-
- return h1s + p
+ fr = data


This needs to be fr = data.copy() or else the next few lines changes the values in the original data array.

Looking into this a bit more a boolean comparison can be used here. I believe the following should suffice.

# Calculation of penalty fr = data < 0 # 0 if y >= 0, 1 if y < 0 pr = kwargs['gamma'] * np.nansum(fr * data**2)

atomman added 2 commits March 4, 2016 13:57

Update proc_autophase.py

a7d4381

The Automatic phase correction did not work as intended. Especially the penalty function was calculated incorrectly. I think. Also there should be an option to set the derivative order and a penalty function scaling factor, gamma.

Update proc_autophase.py

cac6925

Needed to take the absolute of the n-derivative of the spectrum

kfritzsc reviewed Mar 4, 2016
View reviewed changes

Update proc_autophase.py

b93dfd7

I have made changes to the autops function to accept the argument for the gamma constant. Also I have set the options to return the phases in a tuple. I'm unsure about how to describe these changes in the doc?

atomman added 2 commits March 12, 2016 19:47

Update proc_autophase.py

5ff1ad1

Replaced autops with functions psacme and pspmin. As the two methods relies on different input the should have their own functions I think.

Update proc_autophase.py

53a0573

atomman closed this Mar 12, 2016

atomman reopened this Mar 12, 2016

jjhelmus reviewed Mar 15, 2016
View reviewed changes

atomman added 3 commits March 16, 2016 19:38

Update proc_autophase.py

5a1fe51

Update proc_autophase.py

197615f

Update proc_autophase.py

bf24986

jjhelmus reviewed Sep 16, 2016

View reviewed changes

Update proc_autophase.py

e61f2ad

forgot autops function in last commit

Update proc_autophase.py

6f49c45

Added a few white spaces and changed a np.sum to np.nansum

jjhelmus reviewed Sep 23, 2016

View reviewed changes

nmrglue/process/proc_autophase.py

def _ps_acme_score(ph, data):

def _ps_acme_score(ph, data, kwargs):

Copy link

Owner

jjhelmus Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**kwargs?

jjhelmus reviewed Sep 23, 2016

View reviewed changes

Update proc_autophase.py

ec4e921

jjhelmus reviewed Oct 5, 2016

View reviewed changes

kaustubhmote mentioned this pull request Jul 11, 2019

Problem with proc_autophase.autops(data, "acme") #102

Closed

kaustubhmote mentioned this pull request Jul 30, 2019

Additional options for autops #108

Merged

kaustubhmote mentioned this pull request Sep 7, 2020

Better proc_autophase.py #124

Merged


		return phasedspc
		phasedspc = ng.proc_base.ps(data, p0=opt[0], p1=opt[1])


		def _ps_acme_score(ph, data):
		def _ps_acme_score(ph, data, kwargs):



		def _ps_peak_minima_score(ph, data):
		def _ps_peak_minima_score(ph, data, kwargs):

Update proc_autophase.py #48

Are you sure you want to change the base?

Update proc_autophase.py #48

Conversation

atomman commented Mar 4, 2016

Choose a reason for hiding this comment

jjhelmus commented Mar 7, 2016

atomman commented Mar 9, 2016

jjhelmus commented Mar 9, 2016

atomman commented Mar 12, 2016

Choose a reason for hiding this comment

jjhelmus commented Mar 15, 2016

chatcannon commented Mar 16, 2016

coveralls commented Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

jjhelmus commented Sep 16, 2016

mfitzp commented Sep 16, 2016 • edited Loading

atomman commented Sep 17, 2016

mfitzp commented Sep 17, 2016 • edited Loading

coveralls commented Sep 20, 2016 • edited Loading

atomman commented Sep 20, 2016

mfitzp commented Sep 20, 2016 • edited Loading

coveralls commented Sep 20, 2016 • edited Loading

atomman commented Sep 20, 2016

coveralls commented Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 23, 2016 • edited Loading

jjhelmus commented Oct 5, 2016

atomman commented Oct 5, 2016

simulate a spectrum with 2 lorentzians and noise

apply linear phase to the spectrum to create 'raw' un-phased spectrum

phase raw spectrum using various techniques

plot spectrums

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 16, 2016 •

edited

Loading

mfitzp commented Sep 16, 2016 •

edited

Loading

mfitzp commented Sep 17, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

mfitzp commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 23, 2016 •

edited

Loading