Clarifications for super-Gaussian windowing

[tomasstolker]

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 window parameter, 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:

• The sigma parameter in super_gaussian is changed window. From what I could tell, the value is the FWHM and not the standard deviation of the super-Gaussian.
• There was a factor 2 in sigma=window * 2 that I found confusing. I think it was there because the window value was used as HWHM instead of the FWHM. The new implementation is such that window is actually the FWHM.
• I have added docstrings to the super_gaussian and apply_windowing functions.
• The window_contours parameter was added to show_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 apod is redundant since setting window=None will 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!

[tomasstolker]

The other option would be that we change the documentation of the window parameter to HWHM. Perhaps that would be better because it would not change the behavior of the cleaning function?

[neutrinoceros]

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.

Suggested change

	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:

[tomasstolker]

I have changed the parameter name back to sigma 👍

[tomasstolker]

In order to not break any existing code, I think it would be best to simply change the documentation of the window parameter (i.e. mention that it is the HWM instead of the FWHM, but please check if I understood correctly), instead of fixing the implementation such that it is the FWHM (i.e. the current PR). Would you agree?

[neutrinoceros]

My expertise is purely technical and I don't know the details of implementation so I'll defer to @DrSoulain for validation and clarification.

requested changes were applied

[DrSoulain]

Hi @tomasstolker, thanks for your PR. Indeed, the window is the HWHM (and not the FWHM, as mentioned in the tutorial). Actually, we used radius to be consistent with the parameters applied for the sky computation (also radius). So, I suggest changing the documentation and, as pointed out, and notify users that the apod parameter will be removed in the next release.

[DrSoulain]

Your modification and clarification on the super_gaussian function or show_clean_parameters are still helpful (thanks). Could you change the PRs to clear conflicts and check not to change the actual behavior of the feature? (window is the HWHM).

[tomasstolker]

Thanks for your feedback! I have changed the use of the window back to the original implementation and simply fixed the description to HWHM. Please have another look. I also added some warnings regarding the deprecation of apod and set the default value to False. That results however in many errors with the tests because warnings are converted into errors. Shall I remove the warnings again from the code? Because it seems to much work to update all the tests for this...

[neutrinoceros]

Because it seems to much work to update all the tests for this...

yeah this deprecation probably needs some additional thinking, we don't want to have self-triggered deprecation warnings !
I would suggest we postpone it to a future PR.

[tomasstolker]

I have commented out the warnings about apod (in case you want to use it for a future PR).

[tomasstolker]

Sure! Then I will leave that to you guys.

[neutrinoceros]

I opened #210 to keep track of this problem.

[tomasstolker]

Hereby a quick reminder about this open PR. I just synchronized it with the main branch. Let me know if you have any further feedback!