autograd support for web.run(component_modeler) #2745
Conversation
yaugenst-flex
changed the title
yaugenst-flex
force-pushed
the
yaugenst-flex/autograd-web-cm
branch
from
8f28ad9 to
198d60c
Compare
There was a problem hiding this comment.
See my comments. The main thing we should discuss is whether running the ComponentModeler from the regular tidy3d.web.run() is worth injecting all of this dependency of the web API on the component modeler logic. I would lean towards just making a plugins.smatrix.web to handle this stuff, and the non-autograd version can just call regular web.run(). We should discuss @yaugenst-flex / @daquinteroflex . What do we gain by using tidy3d.web.run()?
| from tidy3d.web.api.autograd import autograd as web_ag | ||
|
|
||
|
|
||
| def _run_emulated_minimal(simulation: td.Simulation, path=None, **kwargs) -> td.SimulationData: |
There was a problem hiding this comment.
could any of this be ported to the regular run_emulated? would be nice to have it in one place.
There was a problem hiding this comment.
these tests need to be reworked altogether, this is basically just in place so that we can have a sanity check that this chain is intact. the emulated run in test_autograd.py is super heavy and if we use it here then the component modeler test takes 10 seconds
| if isinstance(simulation, ComponentModelerType): | ||
| try: | ||
| sims = simulation.sim_dict | ||
| except Exception: |
There was a problem hiding this comment.
when does this come up? Seems like something we should not handle silently?
| raise AdjointError( | ||
| "No frequency-domain data found in simulation, but found traced structures. " | ||
| "For an autograd run, you must have at least one frequency-domain monitor." | ||
| def _is_valid_sim(sim: td.Simulation) -> bool: |
There was a problem hiding this comment.
one potential thing we could add is a private property _is_valid_for_autograd() to a few classes, such as Simulation and ComponentModeler and then just call that here. That would separate out this logic specific to the component modeler
|
Basically propagated latest changes from the PR branch #2755 I also updated the implementation a bit so that all web imports would be at the same file scope, as ideally was targeting no more web imports at any of the base classes and only can be done inside on-the-fly function calls to avoid the web circular import problem. |
* Update web status * Minor fixs * Final typo
* Update types structure * Fix precommit * Reorganise based on feedback * Revert "Reorganise based on feedback" This reverts commit 6aeb51e. * Revert "Revert "Reorganise based on feedback"" This reverts commit 332ce71. * fix cycles & type imports * revert some formatting changes * fix relative imports * another round of fixing --------- Co-authored-by: Yannick Augenstein <[email protected]>
* Update web status * Minor fixs * Final typo * Now need to break the loop * Downloading
* Update to new data name * Remove polluting scope * no issue mixing import scope on run.py * Update tidy3d/plugins/smatrix/component_modelers/base.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Readd PayType to run.py * Correct fix --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
daquinteroflex
force-pushed
the
yaugenst-flex/autograd-web-cm
branch
from
7f01c56 to
d5c0555
Compare
|
Just to add a future TODO we need to properly fix d5c0555 |
daquinteroflex
force-pushed
the
dario/rf_endpoints_update
branch
8 times, most recently
from
9321f2b to
1745219
Compare
No description provided.