-
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
auto grab permittivity inside and outside PolySlab
and Cylinder
#2080
Conversation
@yaugenst-flex this is causing the |
it's because the coordinates are outside of the interpolation range,: tidy3d/tidy3d/components/autograd/derivative_utils.py Lines 165 to 168 in 06a513d
where
this can be avoided by setting not sure if that is the correct solution though, probably the coordinates should be aligned before interpolation in this case. or we use |
hm, ok I'll try this. is there a good notebook to test? |
946283d
to
79c4bd0
Compare
PolySlab
9f4c0eb
to
e2e659d
Compare
PolySlab
PolySlab
and Cylinder
OK, so I ended up with this:
I think this is best of both worlds, because it leaves Box alone and enables PolySlab to work properly. Tested with PhC and Metalens notebooks. |
structures=structs_inf_struct, | ||
medium=structure.medium, | ||
monitors=[], | ||
) |
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.
Calling updated_copy
on simulations always throws a red flag for me in terms of efficiency, because the simulation will get re-validated, which in some cases can be slow. In this particular case you're also doing this for every structure. I suspect this is going to get quite heavy e.g. in a metalens case where you have many structures so both re-validating each time may be costly, and it will be done many times (so at least O(N**2) scaling?)
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.
I think the answer here may be to refactor the Simulation.epsilon
internals to have a function that can just get the epsilon of a list of structures without having to copy the simulation.
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.
Also computing over the entire simulation grid sounds like a giant overkill right? I mean, maybe we can't do it close to edges only, but at least within the structure bounding box + some spacing?
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.
- It will be a performance hit for sure. We have plans to have an "autograd config" where we can toggle this on and off (off by default) depending on if the user needs this extra accuracy.
- For things like metalens, it's likely to be a single structure since it's a geometry group. For example I tried this for both metalens and phc notebooks and it was negligible.
- We can try refactoring simulation.epsilon, maybe adding a way to skip certain structures? for the inside one, I need to add the structure.medium to Simulation.medium, or make it infinitely large, which will require some modification.
- It would be worth also considering adding a
validate: bool
argument toupdated_copy
? I thought Weiliang implemented this somewhere recently. - The epsilon is not computed over the entire simulation, just over the region defined by the
.geometry
of the graidient permittivity monitor.
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.
Interesting, thanks for the clarification!
For things like metalens, it's likely to be a single structure since it's a geometry group. For example I tried this for both metalens and phc notebooks and it was negligible.
I guess that's what we try to teach people to use, but they don't always - but fair point! So the inside and outside eps will be computed only once?
It would be worth also considering adding a validate: bool argument to updated_copy? I thought Weiliang implemented this somewhere recently.
You're right, this is currently on pre/2.8 only. Actually should we put this in 2.8 too just to avoid unexpected side-effects? It seems like there could be some since who knows what edge cases we haven't thought about.
The epsilon is not computed over the entire simulation, just over the region defined by the .geometry of the graidient permittivity monitor.
Ah ok I see.
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.
[For geometry group] So the inside and outside eps will be computed only once?
yes
Actually should we put this in 2.8 too just to avoid unexpected side-effects?
I guess it's up to you, but that would be safer yea. Maybe I should build in some logic to preferentially skip this calculation if the user still manually supplies background_medium
.
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.
I thought that's what's gonna happen currently?
Yeah I think we should do 2.8 otherwise we're risking having to do another patch like few days later when we figure out some issue with this. :D
But wait for my merge before you change the base branch and cherry-pick this.
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.
ok, no rush
oops, accidentally force pushed and erased your merge commit @yaugenst-flex |
yeah no problem that one was supposed to go anyways |
c66d0e0
to
e3186d3
Compare
this has been rebased and tests fixed FYI |
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.
Fine by me to merge after squashing the commits. @yaugenst-flex you good on it too?
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!
a8fc9be
to
f1c11d7
Compare
this needs a bit more testing, dont merge it yet (just FYI) |
df4afa5
to
e2fd9ba
Compare
ok I think this is ready to go. tested on a handful of notebooks with different settings and it works. Btw here's the logic for how the outside permittivity is set if Box:
if PolySlab or Cylinder
|
Sounds good to me! |
Fixes #1121
Approach is to call
Simulation.epsilon
without the structure present and then store this information in theDerivativeInfo
. Then the gradient calculation can interpolate this data where it's needed, which gives a pretty good prox y to the permittivity outside the structture at that location.Permittivity inside can still just use the
Structure.medium.eps_model
Implemented for
PolySlab
but not yet forBox
.