NanoComp / meep

free finite-difference time-domain (FDTD) software for electromagnetic simulations
GNU General Public License v2.0
1.23k stars 622 forks source link

Backward propagating eigenmode adjoints are wrong #1585

Open smartalecH opened 3 years ago

smartalecH commented 3 years ago

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:

bw = mpa.EigenmodeCoefficient(sim, source_vol, kpoint_func=lambda a,b: mp.Vector3(x=-1))

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).

stevengj commented 3 years ago

Test case: image

ahoenselaar commented 3 years ago

There is certainly an extremely dangerous coding practice in use here: https://github.com/NanoComp/meep/blob/master/python/adjoint/objective.py#L34

Going from a boolean 'forward' to an integer 'self.forward' with the opposite boolean interpretation is asking for trouble. forward=True becomes self.forward=0 (interpreted as False in a Boolean context).

smartalecH commented 3 years ago

There is certainly an extremely dangerous coding practice in use here

Agreed but not the cause of the above issue, luckily (in case anyone was wondering).

ahoenselaar commented 3 years ago

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.

ianwilliamson commented 3 years ago

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.

ahoenselaar commented 3 years ago

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 https://github.com/NanoComp/meep/issues/1585#issuecomment-854867137, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4VK57DXJEVHNZR5MTZXKLTRD7HFANCNFSM455ZFBRQ .

oskooi commented 3 years ago

The cause of this issue is described in #1773 (comment). The fix involves (1) modifying the EigenmodeCoefficient class to use the centered grid rather than the Yee grid for the DFT mode monitors and (2) using a large enough Meep grid resolution such that the discretization error due to the interpolation of the eigenmode from MPB into Meep is negligible.