Skip to content

feat(tidy3d-notebooks): GaussianBeam source gradient demonstration#472

Open
groberts-flex wants to merge 3 commits intodevelopfrom
groberts-flex/gaussian_source_gradients
Open

feat(tidy3d-notebooks): GaussianBeam source gradient demonstration#472
groberts-flex wants to merge 3 commits intodevelopfrom
groberts-flex/gaussian_source_gradients

Conversation

@groberts-flex
Copy link
Copy Markdown
Contributor

This notebook demonstrates optimizing GaussianBeam source gradients for a grating coupler alongside geometric parameters to target designs that are robust to source/packaging misalignments.

@marcorudolphflex
Copy link
Copy Markdown
Contributor

Cool demo, thanks!
Some room for improvement:

  1. Check CI-relevant lint/spellcheck
  2. Add thumbnail image, add in metadata
  3. Index the notebook in docs/features/autograd.rst
  4. I see big vertical gaps in grating coupler design comparisons - let's save space here and make it better comparable
  5. I would add some more references to other grating couplers and autograd tutorial notebooks.
  6. rename to Autograd31... (parallel adjoint notebook is now Autograd30)
  7. cell starting with def to_norm(value_phys, min_val, max_val): is very big and should be split semantically (like plotting/optimization/simulation setup) with some more descriptive md cells added in between. Would also add more comments or docstrings for some functions. We could also consider to move the "more interesting" or relevant functions like the "run..." ones to the cells where they are actually used as this is code to pay more attention to for the users. For the "boring" plot helpers, I would say it is okay to just define and hide all of them in one big cell with less explanation. Still I would let pay the users pay more attention to the "interesting" helpers with APIs which are actually relevant to handle with.
  8. Can we explain the big dip in the Frozen-source optimization history? Is that more smooth with a smaller learning rate?
  9. I think it's quite unusual in our notebooks to use ALL_CAPS variables? Consider making them lowercase. Also I would add some explanatory comments to most of them. Would move the plot-related variables (and the plot.rcParams calls) to the plotting helper cell.
  10. use tidy3d adam instead of optax adam
  11. remove local_gradient=True (or the local gradient option at all) - we should release this notebook after the tidy3d release of this feature anyways - or is it already released?
  12. rename task_names starting with autograd35 - maybe remove this prefix completely to make this safe against some future migration
  13. optional: log grad norm over optimization steps to get better feeling for convergence?
  14. Use lower-case in title
  15. I would make the introduction a little bit more detailed by introducing the concept how we will archieve with some more detail - I could imagine users would not directly get those short explanations like "improves robustness only through geometry changes"?

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.

2 participants