Skip to content
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

Open
smartalecH opened this issue Jun 2, 2021 · 7 comments
Open

Backward propagating eigenmode adjoints are wrong #1585

smartalecH opened this issue Jun 2, 2021 · 7 comments

Comments

@smartalecH
Copy link
Collaborator

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 stevengj added the bug label Jun 2, 2021
@stevengj
Copy link
Collaborator

stevengj commented Jun 2, 2021

Test case:
image

@ahoenselaar
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Contributor

ianwilliamson commented Jun 4, 2021

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
Copy link
Contributor

ahoenselaar commented Jun 4, 2021 via email

@oskooi
Copy link
Collaborator

oskooi commented Oct 25, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants