Clarifications for super-Gaussian windowing #202
Conversation
…arameter to show_clean_params
for more information, see https://pre-commit.ci
|
The other option would be that we change the documentation of the |
amical/tools.py
Outdated
| def super_gaussian( | ||
| x: np.ndarray, window: float, m: float = 3.0, amp: float = 1.0, x0: float = 0.0 | ||
| ) -> np.ndarray: |
There was a problem hiding this comment.
Annotations are cool, but parameter names cannot be changed arbitrarily for keyword-allowed arguments (sigma -> window), since that would break user code calling the function with sigma passed as keyword.
| def super_gaussian( | |
| x: np.ndarray, window: float, m: float = 3.0, amp: float = 1.0, x0: float = 0.0 | |
| ) -> np.ndarray: | |
| def super_gaussian( | |
| x: np.ndarray, sigma: float, m: float = 3.0, amp: float = 1.0, x0: float = 0.0 | |
| ) -> np.ndarray: |
There was a problem hiding this comment.
I have changed the parameter name back to sigma 👍
|
In order to not break any existing code, I think it would be best to simply change the documentation of the |
|
My expertise is purely technical and I don't know the details of implementation so I'll defer to @DrSoulain for validation and clarification. |
|
Hi @tomasstolker, thanks for your PR. Indeed, the |
|
Your modification and clarification on the |
… deprecation of the apod parameter
for more information, see https://pre-commit.ci
|
Thanks for your feedback! I have changed the use of the |
yeah this deprecation probably needs some additional thinking, we don't want to have self-triggered deprecation warnings ! |
|
I have commented out the warnings about |
|
Sure! Then I will leave that to you guys. |
|
I opened #210 to keep track of this problem. |
This PR addresses issue #195 and adds clarification regarding the super-Gaussian windowing that is applied with the data preprocessing.
If I understand correctly now (please correct me otherwise!), I was mainly confused by the
windowparameter, which was actually the HWHM instead of the FWHM. The latter is how it is descriped in the docs.I have made some changes to the implementation:
sigmaparameter insuper_gaussianis changedwindow. From what I could tell, the value is the FWHM and not the standard deviation of the super-Gaussian.sigma=window * 2that I found confusing. I think it was there because thewindowvalue was used as HWHM instead of the FWHM. The new implementation is such thatwindowis actually the FWHM.super_gaussianandapply_windowingfunctions.window_contoursparameter was added toshow_clean_params(default:False) to show several contours of the window functions. I found this useful in addition to only showing the circle for the FWHM.One other thing that I noticed is that the
apodis redundant since settingwindow=Nonewill have the same effect. I did not change this but removing the parameter may simplify the list of input parameters a bit.I hope this is helpful and let me know if you have any feedback or questions!