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

Added two-photon absorption nonlinearity #1172

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Added two-photon absorption nonlinearity #1172

merged 1 commit into from
Nov 6, 2023

Conversation

caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Sep 25, 2023

  • Adds TwoPhotonAbsorption and NonlinearModel classes. Also, nonlinearities of different types can be combined. Note that the beta and n2 parameters for these classes can be complex. Also, these classes need n0, which can be provided or obtained automatically from the medium and source frequencies.
  • Adds warning for normalizing against CustomSourceTime or ContinuousWave sources

@caseyflex caseyflex marked this pull request as draft September 25, 2023 14:10
@caseyflex caseyflex force-pushed the casey/tpa branch 3 times, most recently from 887bad1 to 28d8e93 Compare September 25, 2023 14:26
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

just a few minor comments as I looked over this.

tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Show resolved Hide resolved
@caseyflex caseyflex changed the base branch from develop to pre/2.5 October 17, 2023 15:00
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Looks good! just a few minor comments.

CHANGELOG.md Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
@momchil-flex
Copy link
Collaborator

I am a tad unhappy with the naming of numiters as opposed to num_iters. Generally we follow the num_*** convention in everything (e.g. num_poles, num_layers... and even the num_iters in the fitter). However, we already released the first iteration of nonlinear stuff in 2.4, so I don't know if we want to make that change. That said however, are you introducing any breaking changes here anyway?

@caseyflex
Copy link
Contributor Author

I am a tad unhappy with the naming of numiters as opposed to num_iters. Generally we follow the num_*** convention in everything (e.g. num_poles, num_layers... and even the num_iters in the fitter). However, we already released the first iteration of nonlinear stuff in 2.4, so I don't know if we want to make that change. That said however, are you introducing any breaking changes here anyway?

I am not introducing any breaking changes here. I can rename numiters to num_iters, and keep the old parameter in NonlinearSusceptibility and make it still work for backward compatibility.

@tylerflex I would like to move num_iters into NonlinearSpec, rather than duplicating it in every model. Would you be ok with this? I know we didn't want too many specific parameters in the general NonlinearSpec, but I feel this one is sufficiently fundamental to move there. And that would save the user from having to type it in each NonlinearModel.

@tylerflex
Copy link
Collaborator

tylerflex commented Oct 18, 2023

@tylerflex I would like to move num_iters into NonlinearSpec, rather than duplicating it in every model. Would you be ok with this? I know we didn't want too many specific parameters in the general NonlinearSpec, but I feel this one is sufficiently fundamental to move there. And that would save the user from having to type it in each NonlinearModel.

As long as you think we can be pretty sure that this will be a field in every nonlinear model we add, specifically that future nonlinear models will not need their own num_iters. If it's just about backwards compatibility, we can easily handle this I think by having num_iters and numiters but adding a validator on numiters which asks users to use num_iters and then just sets it with the supplied value. Also alias might be worth looking into. We might be able to set a field num_iters with an alias of numiters.

https://docs.pydantic.dev/latest/concepts/alias/

@caseyflex caseyflex force-pushed the casey/tpa branch 2 times, most recently from 9232d2d to 4c93834 Compare October 18, 2023 16:54
@caseyflex
Copy link
Contributor Author

@tylerflex I added KerrNonlinearity as well as a note about how that relates to chi3

@momchil-flex momchil-flex added the rc3 3rd pre-release label Oct 30, 2023
@caseyflex caseyflex force-pushed the casey/tpa branch 4 times, most recently from cde87aa to 9df4841 Compare October 31, 2023 18:37
@caseyflex caseyflex marked this pull request as ready for review October 31, 2023 18:37
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Thanks @caseyflex, pretty minor comments. Still a little confused about the numiters behavior so asked for some clarification (and potentially a strange behavior spotted in a validator), but otherwise looks good.

tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
@momchil-flex momchil-flex merged commit ebae633 into pre/2.5 Nov 6, 2023
12 checks passed
@momchil-flex momchil-flex deleted the casey/tpa branch November 7, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rc3 3rd pre-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants