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

more polyslab gradient bug fixes #2020

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Conversation

tylerflex
Copy link
Collaborator

Some minor but important polyslab gradient fixes. Surprised I missed this but glad I ended up catching it after all. Goes to show how tricky these things can be.

  1. Found a sign error in the gradient contribution. This was not really affecting anything except in certain polarizations, it turned out to improve a customers' taper gradient accuracy, and fixed one of my numerical tests under certain conditions.

  2. Also realized some condition for incorporating the slab_bounds into the area calculation was not being included as it was out of date with another PR. So after this fix + another tweak to the gradient norm, I am seeing POlySlab gradients matching numerical not only better than before but with the right magnitude.

@tylerflex tylerflex marked this pull request as draft October 16, 2024 21:04
@tylerflex tylerflex marked this pull request as ready for review October 17, 2024 14:22
@tylerflex
Copy link
Collaborator Author

I tested this a bit more (as mentioned) and I think it's good to go.

@tylerflex tylerflex force-pushed the tyler/autograd_/fix/polyslab2 branch from bb7e202 to 1b30214 Compare October 22, 2024 15:28
@tylerflex tylerflex force-pushed the tyler/autograd_/fix/polyslab2 branch from 54959aa to 6b254a7 Compare October 22, 2024 15:58
@tylerflex
Copy link
Collaborator Author

tylerflex commented Oct 22, 2024

OK this is ready, freshly tested in notebooks and all seems good.

The changes:

  1. Fixed a sign in the D field contribution to polyslab vertices gradient.
  2. Included the slab_bounds in the differential area calculation for vertices adjoint, which was not being taken into account because of a condition that no longer was being triggered after earlier changes.
  3. Fixed the norm of the FieldMonitor adjoint source for both point and volume monitors.
  4. Fixed the phase of the DiffractionMonitor adjoint source, which is consistent with adjoint now and works for all polarizations and layer thicknesses in our notebook.
  5. Revert an earlier "fix" to Box epsilon grabbing that made things worse

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If testing shows that the gradients are better, I trust that. It's really hard to make sure that all these phases, etc. are accounted for correctly in any other way but testing.

@@ -1037,7 +1037,7 @@ def to_adjoint_point_sources(self, fwidth: float) -> List[PointDipole]:

for freq0 in field_component.coords["f"]:
omega0 = 2 * np.pi * freq0
scaling_factor = 1 / (MU_0 * omega0)
scaling_factor = 33 / (MU_0 * omega0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that seems pretty arbitrary :D why 33?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the honest answer is because I tested the PointDipole sources against every structure combination and the gradients were consistently 33x smaller than numerical ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little worrying then. Have you tested different grid sizes at the monitor location? And the monitor being in different materials?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to test everything, I already had like a 10 x 4 grid of different combinations of things to test, but no I did not test grid size and medium. The grid size should be handled by using interp in the source, the medium could have an effect but I think it would be some scaling by the refractive index, the overall constant might still be the same. I can look into it some more if it would be useful

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm just trying to think about things that this "constant" might actually depend on, like the mesh step around the dipole for some reason, or the material epsilon.

Copy link
Collaborator Author

@tylerflex tylerflex Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's very possible the gradients depend on these things in some way. This PR resolve the gradient direction and corrects the overall normalization so that the un-normalized gradients match pretty well (typically under 10% unnormalized and around 1% normalized), but under my test conditions. So There's another dimension to explore which is how these norms change under different background permittivity and grid resolution.. which gets pretty complicated. I think for sake of practical optimization what we have here is a good improvement, but I'll do a few more tests today to see if I can find some low hanging fruit to improve it

Copy link
Collaborator Author

@tylerflex tylerflex Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested a few different Simulation.medium and also a few different grid sizes. The grad norms were constant across each, maybe a tiny bit of variation with dl, but didnt seem linear with dl and could be due to many things. Ultimately feel confident that these constants are good to give adj ~ numerical grads without normalization for a range of situations, and if not, we can revisit later. So I'll go ahead and merge this

@tylerflex tylerflex merged commit 112c059 into develop Oct 23, 2024
15 checks passed
@tylerflex tylerflex deleted the tyler/autograd_/fix/polyslab2 branch October 23, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants