-
Notifications
You must be signed in to change notification settings - Fork 49
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
refactor gradient calculations to use DerivativeSurfaceMesh
#2056
Conversation
c4a9272
to
e721e9b
Compare
ad260c1
to
a644b17
Compare
Let's also get this one in. Looks like I have a good amount of merge conflicts to sort out first, but I'll work on that today and re-test |
6778774
to
a9cb96e
Compare
did a bit of testing of this and it seems like it still works. going to leave it unsquashed for now but should be ready to go after reviews @momchil-flex @yaugenst-flex |
I just noticed an issue with the epsilon grabbing from the other PR after the rebase, so don't merge this yet as I'll fix it tomorrow morning |
alright fixed it, turned out when there were |
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.
Really nice to have this 🎉
Only some minor comments. Biggest issue for me is, this is basically a merge from develop right? I think we should do that separately, there are a lot of unrelated changes in here.
Yeah @tylerflex I think you need to drop the first 6 commits (then maybe clean up the rest?) |
f3d8e79
to
3cb0cfd
Compare
ok I cleaned up the commits into just 2, addressed Yannick's comments, but now ruff is complaining (but not locally, so that's a bit weird) |
3cb0cfd
to
144ab3e
Compare
@@ -248,34 +248,25 @@ def get_monitor_name(index: int, data_type: str) -> str: | |||
return monitor_name_map[data_type] | |||
|
|||
def make_adjoint_monitors( | |||
self, freqs: list[float], index: int | |||
self, freqs: list[float], index: int, field_keys: list[str] |
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.
Whenever changes are made to the frontend autograd components we need to be careful about the backend too. Seems like this needs to be updated and I'm not sure how?
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.
ah, right. I think the fix is to pass the sim_fields_keys
into the make_adjoint_monitors
. Since you have the optimizer, that would mean getting
sim = optimizer.design.simulation
traced_fields_sim = setup_run(simulation=sim)
sim_fields_keys = list(traced_fields_sim.keys())
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 don't think setup_run
will work if this is not called inside a grad
call?
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.
not sure, guess you could just do
return simulation.strip_traced_fields(
include_untraced_data_arrays=False, starting_path=("structures",)
)
which is what setup run does.
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.
But the point is that there are no traced fields since it's not a simulation called in grad
no?
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.
Basically what we're trying to do there is get the gradient monitors from the static definition of the InverseDesign object.
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.
ah ok. yea I guess we dont know. but we can effectively "fake" it then, because we know it's a Box with a CustomMedium and we need the permittivity
data.
So I think one approach could just be to pass
field_keys=(f'structures/{(len(sim.structures) - 1}/medium/permittivity', )
into the make_adjoint_monitors
call. Or you can just make the monitors manually since again we know it's just a box so it needs a similarly sized field and 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.
Using the first approach, works for now, obviously should be dealt with better in the future..
144ab3e
to
e582bbc
Compare
Part 1 of an effort to generalize the VJP calculations so the Geometry and Medium just need to construct a single
DerivativeSurfaceMesh
for every field. TheDerivativeInfo
then has a method that computes derivatives given aDerivativeSurfaceMesh
.