-
Notifications
You must be signed in to change notification settings - Fork 15
S Matrix Autograd #341
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
S Matrix Autograd #341
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Here my comments:
- I know it's topologically the same thing, but this is a very strange looking waveguide crossing 😆 Might be quite confusing to readers to have the ports arranged like that..
- You actually don't use autograd numpy other than for setting up to (undifferentiated)
linspace
s - I guess it works, which is nice. But I'd be careful and just use autograd numpy throughout? - Before Cell 15: Typo:
optimizer.th
- Cell 18: You set
beta_min = beta_max = 20
, so no beta scheduling. Is that intentional? If it is, it probably should be commented. - Cell 18:
params
get appended toparams_history
and are always mutated in-place, meaning that the finalparams_history
will contain the same parameters for each entry. Should beparams_history.append(params.copy())
. import tidy3d.web as web
unused. Similarly, there are a couple of other formatting issues, should runruff format && ruff check --fix
. CI is failing.
can you help me make it look better? how would you change it? |
sounds good, fixed
fixed
yea it seems to work alright, will add a comment
fixed
thanks, fixed this too Will change to a "Plus" setup and will re-run but the queue is horrible today so it's like an iteration every 45 minutes.. |
Spell Check ReportAntennaCharacteristics.ipynb:
Autograd26Smatrix.ipynb:
EdgeFeedPatchAntennaBenchmark.ipynb:
WPDHarmonicSuppression1.ipynb:
WPDHarmonicSuppression2.ipynb:
WPDHarmonicSuppression3.ipynb:
Checked 13 notebook(s). Found spelling errors in 6 file(s). |
thanks Tyler, this is really cool! A couple of comments below
at cell 21, seeing that the output is broken which I'm assuming is due to all the flux in the backend and how the autograd stuff is running. When that part is updated and re-run, I'll add some more comments for the rest of the notebook |
Thanks @groberts-flex , I made all comments and fixed the element mappings too so it only solves from one port (since all results can just be rotated through the ports). Sorry you saw this version, I had made some major changes after Yannick's initial reviews so it was a bit messy. but appreciate the help cleaning it up. Once things are back up and running I will polish up and get things into final state. |
oh another comment i just remember: we should use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown above [9]: "parmeterization"
[9]: functools.partial
is unused now right?
[14]: Nit, but the dtype=float
isn't necessary. It'll be promoted in the objective function, so not including this might improve readability a bit.
[15]: Remove commented out import.
[17]: Lots of ipywidgets
warnings. Might be nice to just install that locally when running the notebook so it doesn't clutter the output here?
[26]: Any reason not to plot this over wavelength? Currently seems somewhat difficult to parse, especially since it's plotting normalized frequency. Also, a before/after plot would be nice..
3d0ed4d
to
deab19c
Compare
Thanks @yaugenst-flex , all suggestions have been addressed. notebook was re-run and for some reason the grey region just went away by itself. Squashed commits. Should be ok to merge now @daquinteroflex |
deab19c
to
e27b3ab
Compare
e27b3ab
to
2d15618
Compare
This PR will merge into pre/2.10 branch btw |
Requires a merge of frontend PR