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

Better proc_autophase.py #124

Merged
merged 3 commits into from
Oct 20, 2020
Merged

Better proc_autophase.py #124

merged 3 commits into from
Oct 20, 2020

Conversation

kivicode
Copy link
Contributor

@kivicode kivicode commented Sep 6, 2020

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

@kaustubhmote
Copy link
Collaborator

@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
Copy link
Contributor Author

kivicode commented Sep 7, 2020

@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
Copy link
Collaborator

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
Copy link
Owner

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.

@jjhelmus jjhelmus merged commit 3813cc5 into jjhelmus:master Oct 20, 2020
This pull request was closed.
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.

3 participants