🗻 autograd: geometry group #1768
Conversation
tylerflex
force-pushed
the
tyler/autograd_/geo_group
branch
from
117eca8
to
5b46485
Compare
tylerflex
added
2.8
tylerflex
force-pushed
the
tyler/autograd_/geo_group
branch
from
a59ab04
to
d9194e6
Compare
|
hey @yaugenst-flex when you get a chance, could you look again with the new added changes to add a |
There was a problem hiding this comment.
Nice I think the DerivativeInfo change is great. Also opens up the possibility for some derivative-specific helpers down the line.
Having a utils.py and a derivative_utils.py feels a bit weird to me, but not sure if it needs changing. I guess we'll merge this and I'll add #1769's interpn to a functions.py here?
| ) | ||
| from .utils import get_static | ||
|
|
||
| # from .derivative_utils import DerivativeInfo, integrate_within_bounds |
There was a problem hiding this comment.
Any reason why this is commented out?
There was a problem hiding this comment.
Because it causes circular imports when this directory is imported into eg base.py. I'll remove the comment though
There was a problem hiding this comment.
Actually i need to double check this. It might be fixed now that i use dict of scalar field data array
| # class passed to gradient computations | ||
| # defined here to make things more compact, extendable, and better documented | ||
|
|
||
| # we do this because importing these creates circular imports |
There was a problem hiding this comment.
I guess this is fine for now but I think we should figure out how to do this more robustly... either reorganizing the types so that this doesn't happen or possibly use forward references.
Or maybe the best way would actually to from __future__ import annotations 🤔
There was a problem hiding this comment.
The issue is that MonitorData such as FieldData can't be imported into eg geometry and medium, where these derivates are computed.
One option might be to extract the derivative computation to somewhere in web/api/autograd instead of being methods of the classes. However, i do kind of like having them defined in the classes as it keeps things organized.
I think annotations will not really help here but could be wrong. Also I'm wary about messing with forward refs unless it's necessary.
There was a problem hiding this comment.
No this should be the exact use case for from __future__ import annotations then 😄 Postponed evaluation of annotations, i.e. you don't need to import the type you're using as a type hint. And it's essentially a forward ref under the hood.
There was a problem hiding this comment.
Yea maybe. I need to read up on forward refs a bit more. Seems like a code smell to me though?
There was a problem hiding this comment.
Practically speaking there was no issue with just passing the field data as generic dicts of data arrays.
| from autograd.tracer import getval | ||
|
|
||
|
|
||
| def get_static(x: typing.Any) -> typing.Any: |
There was a problem hiding this comment.
Is this really necessary? Why don't we just use getval directly? If it's because of naming, we could also from autograd.tracer import getval as get_static here?
There was a problem hiding this comment.
Yea so originally this had some special handling for other types before i learned about getval. I'd say for now let's leave it in case we need to modify things? But probably in the future we can just use getval everywhere we use getstatic
Yea it cleans things up for sure and now i feel less resistance to adding many more options eg for resolution for the eventual cylinder integrals or other things.
Yea it seems a bit weird to me too. Wasn't sure what to rename it to. If you add interpn maybe we can move these to functions.py. Or maybe there's a more descriptive name? I'm fine with it for now |
tylerflex
force-pushed
the
tyler/autograd_/geo_group
branch
2 times, most recently
from
6494018
to
4bdd04f
Compare
tylerflex
force-pushed
the
tyler/autograd_/geo_group
branch
from
4bdd04f
to
82ee8f8
Compare
See example
https://github.com/flexcompute/tidy3d-notebooks/blob/b33c5304deec54cbcf68a6ada8b6ca489e9a6123/Autograd7Metalens.ipynb