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

Remove omega0 from explicit inclusion in optical_element #323

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

rob-shalloo
Copy link
Member

@rob-shalloo rob-shalloo commented Nov 18, 2024

I've made a suggested modification to the optical_element class to remove omega0 from explicit inclusion in the method amplitude_multiplier.

I find it somewhat confusing that we have both omega and omega0 as input arguments. Seeing them both, I would automatically assume that the frequency axis omega is then actually centered around 0, and that omega is rather $\Delta \omega$ such that for each optical element we would need to use omega + omega0 but this is not the case. Instead all but one of our optical elements do not use omega0 at all.

I understand that this was specifically introduced in #261 for the case of polynomial spectral phase #263 . Here I believe we should instead add omega0 as an argument required for initialisation of this optic. This optic adds spectral phase as a polynomial expansion of frequency and so defining the central frequency explicitly is, I think, a good idea.

In any case, as mentioned this is just a suggestion so totally fine to not make the change if people prefer it the way it is. Everything mechanically works great as is

@soerenjalas , could you take a look and see what you think, given you created the two PRs above

Copy link
Contributor

@MaxThevenet MaxThevenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That looks good to me.
@soerenjalas any comment?

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.

2 participants