-
Notifications
You must be signed in to change notification settings - Fork 44
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
🗻 autograd: broadband objectives with single adjoint simulation #1830
Conversation
f273a6e
to
d279e19
Compare
c16de28
to
12725d5
Compare
@momchil-flex @yaugenst-flex sorry for the "bait and switch". I just realized ultimately it makes more sense to unify the multi-frequency under one single approach rather than have various paths. So what I'm doing now is this: The vip-containing I sort all of the adjoint sources by source spatial dependence (eg. I then compute the coupling through Then I solve a linear system with least squares to find a new set of amplitudes to inject with each source. I also turn off To avoid numerical issues, after collecting all of the adjoint sources, I normalize them with the After the adjoint sim is run, the adjoint fields are multiplied by this Note: I have NOT implemented yet:
I'll continue testing next week, and when it seems ready will release it for review, thanks! |
aa0ef00
to
78b0dc7
Compare
OK This PR is ready for review again. Here's how it handles multi-frequency adjoint problems: if the number of frequencies in the objective function is 1, it just does what we did before. if the number of "ports" in the objective function (to put another way, if we can formulate a broadband adjoint source with one source). We do broadband adjoint with a single source and then post-normalize the adjoint fields using the vjp values. Otherwise, we would use other more complicated approaches, in this case, I raised a NotImplementedError. I'm still writing the two demos
Should be done tomorrow or thursday. |
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.
Cool not much to say other than the comments that I left!
ada469b
to
83e5984
Compare
tidy3d/components/data/sim_data.py
Outdated
json_to_sources[src_spatial_json] = src | ||
spatial_to_src_times[src_spatial_json].append(src.source_time) | ||
|
||
num_ports = len(spatial_to_src_times) |
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.
So you are excluding the SourceTime
of all adjoint sources, and counting their unique definitions after that exclusion?
In some cases, like when differentiating through FieldData.flux, the adjoint source would be a CustomFieldSource, right? Does this work for those 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.
So you are excluding the SourceTime of all adjoint sources, and counting their unique definitions after that exclusion?
That's right.
In some cases, like when differentiating through FieldData.flux, the adjoint source would be a CustomFieldSource, right? Does this work for those too?
Good question. I'm not really sure to be honest. Right now it would treat two otherwise identical CustomFieldSource with different data as separate "ports". I think this is the proper handling though, since the data in the source defines the spatial dependence of J(r, ω)
and we're trying to do here is essentially separate J(r)
and J(ω)
, so two different datasets should be treated as different J(r)
I think.
The only case I think this could become slightly ambiguous for the user is if they had two identical FieldMonitor
"a
, and b
and then for some reason a
and b
had different contributions to the objective function. In that case, we could come up with a scenario where the adjoint sources for a
and b
could theoretically be combined into one (ie by just summing the datasets with the corresponding source time amplitudes). But this seems like such an edge case that it's probably not worrying about to me.
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.
just to clarify, in the case described above, we would simply require a separate adjoint simulation for a
and b
, but it wouldn't error in an unexpected way.
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.
My worry was rather that I'm still not sure what happens if you look at the json only, i.e. if the actual data gets compared or just a dummy placeholder tag or something that would be identical for all custom sources. If it works the way you describe it sounds good.
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.
Hm.. actually the way it is set up, I suppose the datasets would be excluded from the jsons. So two identical FieldMonitors a and b would actually be considered the same spatial dependent AdjointSource, and therefore would potentially confuse the broadband adjoint. I'll probably need to look into testing this as an edge case. but for otherwise different FieldMonitor definitions, it will have no problem.
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.
Yeah, that's the edge case I had in mind. Although... technically if they're identical in their definitions, the recorded datasets should also be the same?
Ah but something weird may happen where the two are used in different ways in the objective function and so the adjoint source currents are different?
Very edgy.
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.
well the recorded datasets would be the same, but what we get back is the VJP not the recorded datasets. so yea if the data is used in two different ways, it wouldn't show up.
I think one way to distinguish spatial dependence this could be to instead make a copy of all of the adjoint sources with the same source time, and then hash these. But then I dont know exactly if the datasets are included in the hash or not..
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.
yea so I gave this a stab (hashing temporary copies of the sources with identical source_time) and it seemed to work well
This should be good enough.
5c71c2d
to
b67435f
Compare
c1e2a89
to
8411b8f
Compare
8411b8f
to
9aa7b65
Compare
No description provided.