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

Question about the super-Gaussian function #195

Open
tomasstolker opened this issue Oct 2, 2023 · 6 comments · May be fixed by #202
Open

Question about the super-Gaussian function #195

tomasstolker opened this issue Oct 2, 2023 · 6 comments · May be fixed by #202

Comments

@tomasstolker
Copy link
Contributor

tomasstolker commented Oct 2, 2023

Hello AMICAL team!

The super-Gaussian function that is used for the windowing of AMI data is typically provided in some sort of form like exp(-r^4/r_0), of what I could find in a few articles.

Sorry if I may have misunderstood, but when I looked at the super_gaussian function in the tools module of AMICAL, the window function seemed somewhat different so I was wondering if this is an adjusted form that works better than the "original" form? Effectively the implementation in AMICAL seems to scale as exp(-r^6) instead of exp(-r^4)?

I wanted to suggest to add a reference and/or a few details about this in the documentation, since this seems differently provided in the AMICAL reference and then also incorrectly adopted into e.g. Blakely et al. 2022. Also, the amplitude a=1 in those two publications seems a typo since the constant should probably be before the exp term?

The description of the window parameter in the docstring seems also not fully accurate since it is described as the FWHM of the Gaussian. I think that it might be the HWHM instead? Or perhaps half of a standard deviation? Since in the argument that is provided to super_gaussian is sigma=2*window. I wasn't fully sure how to interpret that. Adding a docstring would also be helpful here.

The actual windowing seems to work fine though 😊 and when I plot the window profile then it looks reasonable (i.e. it has a sharp slope around the specified window value).

I would be happy to create a PR if that would be helpful. Just let me know in that case.

@vandalt
Copy link
Contributor

vandalt commented Oct 2, 2023

Hi @tomasstolker

I'll let @DrSoulain answer the technical and documentation parts, but I just wanted to point share that I had looked into this a while ago.

I compared the super-gaussian window in xara, which is a simple np.exp(-(r/radius)**4) to the one in AMICAL and got the following plot. So the m parameters can be set (if I remember correctly) to get exp(r^4), but I might be wrong. In any case, a plot light this might be useful for documentation, to have a quick intuition of how one can get an equivalent to simpler window functions.

image

@tomasstolker
Copy link
Contributor Author

Thanks @vandalt! Indeed the AMICAL windowing has a flatter top and sharper tapering. The exponent is set to m=3 internally in AMICAL but I don't think it is a user parameter. Did you by any chance also compare the impact of the window shape on the closure phases? I would be curious to know if certain profiles would work better than others.

@vandalt
Copy link
Contributor

vandalt commented Oct 6, 2023

Did you by any chance also compare the impact of the window shape on the closure phases?

I did not yet, but it's on the TODO list for both AMI and Kernel Phase. Maybe @DrSoulain or @kammerje have done this? I know @fmartinache suggested top-hat windows as an alternative, but I did not get to this yet either.

I can share what I find here once it's done. Or maybe a Github Discussion or the Fizeau Hackathon Slack are better suited for this kind of discussion?

@DrSoulain
Copy link
Collaborator

Hi @vandalt and @tomasstolker,

Thank you for bringing this to our attention. We initially implemented the super-Gaussian profile with a fixed 'm' value at the project's start, and at that time, the results from our simulated tests seemed satisfactory, so we didn't investigate further. The 'm' value likely came from the original IDL code.

If you want, you can open a PR, and we can implement a solution to add the 'm' value as expert user parameters and do some tests.

We have a Slack channel for discussions about AMICAL between developers and users. I'd be happy to include you in it for further collaboration and discussions.

@tomasstolker
Copy link
Contributor Author

Thanks for your reply! I would be interested to join the Slack channel.

Happy to create a PR. I think that the implementation of the super-Gaussian is fine. The documentation is just not fully clear/accurate.

@vandalt
Copy link
Contributor

vandalt commented Nov 9, 2023

Thanks for the feedback @DrSoulain. Regarding the Slack, is this the AMI one I'm already on? If not I'd be interested to join too!

@tomasstolker tomasstolker linked a pull request Dec 13, 2023 that will close this issue
@DrSoulain DrSoulain linked a pull request Jan 3, 2024 that will close this issue
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 a pull request may close this issue.

3 participants