jjhelmus / nmrglue

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

Better proc_autophase.py #124

Closed kivicode closed 3 years ago

kivicode commented 3 years ago

Peak width parameter for "peak_minima" phase correction and more human-friendly error message when a wrong key passed.

kaustubhmote commented 3 years ago

@kivicode, this looks like a good addition when phasing the data with either too many peaks, or when the data itself is small. The only comment I have is to switch the position of the return_phases and peak_width arguments in the autops function, so that this doesn't break the code for someone who might be using this function with only positional arguments. Other than that, this looks good to be merged.

There is the question of having an algorithm-specific argument in the general autops function, and there has been a discussion about how to go about doing this in #48. Potentially, one could go about extracting algorithm-specific arguments from kwargs and only passing the remaining ones to fmin. This one looks more "future-proof", so maybe I'll wait for @mfitzp or @jjhelmus to comment on their preference.

kivicode commented 3 years ago

@kaustubhmote Yeah, you're right, I'll swap them.

And how about making a separate dict or tuple variable, that will receive all the additional arguments to pass to fmin's args param. This solution will make it possible to use variables instead of hardcoded values for a custom optimization function.

For example: autops(..., args={"gamma": 42}, ...) or autops(..., args=(42, 73), ...) (not shure about naming for now)

kaustubhmote commented 3 years ago

This looks like a nice solution to me. However, a dictionary wont work, at least for Python < 3.6, since the order is not conserved (and since the fmin function does not take keyword arguments). I think a list with None as the default value will be the best solution at this point. I suggest fn_args as the name. The default number 100 for the peak_width should then be moved to _ps_peak_minima, along with and the docstring should also be appropriately modified. This may be a good way to take care of #48 as well.

jjhelmus commented 3 years ago

Thanks for putting this together @kivicode and for the review @kaustubhmote. This looks like a good change to me. I agree with @kaustubhmote comments that dealing with algorithms specific arguments in autops would be a good addition in the future but nothing something that is needed. here.