-
Notifications
You must be signed in to change notification settings - Fork 50
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 NonlinearSusceptibility with chi3 support #992
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @caseyflex, I think both of these will be super exciting to have. I suggest having @momchil-flex chime in specifically on how we do interpolation of the custom source time as I'm a bit concerned about edge cases using nearest interpolation. Also, is it possible to split this into two PRs? they seem quite separate no? or at least two commits if possible. Otherwise it will be a bit confusing to have both somewhat orthogonal changes in a single commit.
Regarding this:
I dont see why not. currently the Kerr Medium assumes linear medium properties of vacuum? or is this a WIP? |
CustomSourceTime moved here: #994 |
Currently, it is a subclass of Medium, so permittivity and conductivity can be set, or take default values of vacuum. I wasn't sure how we wanted to make it be either a Medium or a DispersiveMedium. So that |
6ef3fdd
to
c3058a0
Compare
9b90b0c
to
7a20ab7
Compare
Hm, maybe we can stick with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me pending two minor rewordings, thanks @caseyflex!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Backend supports dispersive KerrMedium but frontend currently does not. Can we make this subclass something that could be either Medium or DispersiveMedium?
This goes back to our discussion on slack today... It really does feel like our medium subclassing is becoming quite a mess. One option is to treat the nonlinear susceptibility (and the time-modulated permittivity in the future @weiliangjin2021 ) as something that can be added to any medium where it's supported, e.g.
td.Medium(permittivity=..., nonlinearity=KerrNonlinearity(chi3=1))
Then we can pick which media have a nonlinearity
Field. In fact this could still be done through subclassing, something like
# this would probably still subclass from some abstract nonlinearity class
class KerrNonlinearity():
chi3: float
numiters: pd.PositiveInt
class AbstractNonlinearMedium(AbstractMedium, ABC):
nonlinearity: Union[KerrNonlinearity, ...]
# now AbstractNonlinearMedium can be added anywhere we support, e.g.
class Medium(AbstractMedium, AbstractNonlinearMedium):
...
class DispersiveMedium(AbstractMedium, AbstractNonlinearMedium, ABC):
...
In the case of nonlinearity, can it pretty much be added on top of any medium, including custom?
Note: this is just a suggestion, not saying you should implement this right now. But we should decide on something (@tylerflex ) |
Whatever way we introduce the nonlinear medium, we should make sure the mode solver issues a warning that the nonlinearity is not used if someone tries a mode solve in a plane that contains a nonlinear medium. I forget, how do we handle 2D media in the mode solver now? |
In principle it makes sense to be able to add nonlinearity to any medium. However, with the way things are set up with subclassing, I think it might make things a bit complicated. My thinking is just make KerrMedium [and eventually KerrDispersiveMedium], let the class type do the work regarding what properties the medium has, but eventually we should just separate all of the categories of effects in the medium:
into their own fields of a Medium class, each determined by their own specs, and handle custom medium by overloading any fields with data arrays. but this seems like a 3.0 change. I think how Casey has it here is good for now and we can always add the nonlinearity fields into the AbstracMedium class without breaking backwards compatibility if we change our minds? |
7f7739f
to
0d1d1cd
Compare
We use |
Yeah, let's save the restructuring for 3.0. I'll leave it this way with |
0d1d1cd
to
49fa5ee
Compare
7a20ab7
to
e57b41a
Compare
0ed4498
to
48af67f
Compare
48af67f
to
9011559
Compare
9011559
to
fb65ddb
Compare
5c70e4f
to
851ac64
Compare
This will be a better way to do things, I can already tell. Thanks @caseyflex ! |
851ac64
to
192a646
Compare
Looks good to me. Can @dbochkov-flexcompute and @weiliangjin2021 take a quick look to make sure this is in line with our 3.0 thinking regarding mediums? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too!
192a646
to
b5fa26a
Compare
I added AbstractNonlinearSpec for convenience later, and I also changed AbstractMedium.nonlinear_spec to take NonlinearSpecType, but then validate it as needed for different medium types. I think this addresses Daniil's concern and is more robust for the future. I also reintroduced the cap on numiters, which was needed based on benchmarking results |
b5fa26a
to
3c2fe06
Compare
3c2fe06
to
d833077
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve pending the units kwarg in pd.Field()
. thanks @caseyflex
d833077
to
e44c99a
Compare
Known limitations: