-
Notifications
You must be signed in to change notification settings - Fork 634
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
Backward propagating eigenmode adjoints are wrong #1585
Comments
There is certainly an extremely dangerous coding practice in use here: Going from a boolean 'forward' to an integer 'self.forward' with the opposite boolean interpretation is asking for trouble. |
Agreed but not the cause of the above issue, luckily (in case anyone was wondering). |
It would be great to have a small repro case with observed behavior and expected behavior. That could eventually be turned into a unit test. |
See the update to the JAX-Meep wrapper test in #1593. |
Great, thank you!
…On Fri, Jun 4, 2021 at 9:44 AM Ian Williamson ***@***.***> wrote:
It would be great to have a small repro case with observed behavior and
expected behavior. That could eventually be turned into a unit test.
See the update to the JAX-Meep wrapper test in:
https://github.com/NanoComp/meep/pull/1593/commits
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1585 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4VK57DXJEVHNZR5MTZXKLTRD7HFANCNFSM455ZFBRQ>
.
|
The cause of this issue is described in #1773 (comment). The fix involves (1) modifying the |
Note that objective functions that depend on the backward propagating mode produce incorrect gradients right now.
We can get around this by calculating the forward coefficient in the backward direction (i.e. flip the coordinate frame of MPB) using something like this:
Since this hack works, it's probably just some trivial sign error in the adjoint pipeline (which is also why I haven't pursued a fix yet...). That being said, we should add a unit test that verifies this (and someday actually fix this).
The text was updated successfully, but these errors were encountered: