Fix: cascaded mzi notebook + tidy3d pluggin bug fixes#685
Open
David-GERARD wants to merge 4 commits intogdsfactory:mainfrom
Open
Fix: cascaded mzi notebook + tidy3d pluggin bug fixes#685David-GERARD wants to merge 4 commits intogdsfactory:mainfrom
David-GERARD wants to merge 4 commits intogdsfactory:mainfrom
Conversation
Contributor
Reviewer's GuideUpdates the cascaded MZI workflow notebook to compute and batch-run s‑parameter simulations programmatically, aligns the Tidy3D plugin with newer Sequence diagram for updated write_sparameters and web.run interactionsequenceDiagram
participant User
participant write_sparameters
participant ComponentModeler as ComponentModeler
participant web
participant Filesystem
User->>write_sparameters: call write_sparameters(component, dirpath, ...)
write_sparameters->>ComponentModeler: create modeler from component and settings
write_sparameters->>ComponentModeler: compute hash via _hash_self()
ComponentModeler-->>write_sparameters: task_name
write_sparameters->>Filesystem: build path_dir = pathlib.Path(dirpath) / task_name
write_sparameters->>ComponentModeler: modeler = modeler.updated_copy()
write_sparameters->>Filesystem: check if cached sparameters file exists
alt cached sparameters exists
write_sparameters->>Filesystem: load npz file
Filesystem-->>write_sparameters: sparameters data
write_sparameters-->>User: return cached sparameters
else no cache
write_sparameters->>web: run(modeler, task_name=task_name, verbose=verbose, path=path_dir / simulation.hdf5)
web-->>write_sparameters: modeler_data
write_sparameters->>write_sparameters: compute s = modeler_data.smatrix()
write_sparameters->>Filesystem: save sparameters to filepath
write_sparameters-->>User: return computed sparameters
end
Flow diagram for updated cascaded MZI notebook sparameter batch simulationflowchart TD
A_start[Start notebook] --> B_defineSimLengths[Define sim_lengths and other parameters]
B_defineSimLengths --> C_buildJobs[Build jobs list for each length in sim_lengths]
C_buildJobs -->|for each length| C1_jobItem[Create dict with component, filepath, layer_stack]
C_buildJobs --> D_callBatch[Call gt.write_sparameters_batch with jobs]
D_callBatch --> E_getFutures[Receive list of simulation futures sims]
E_getFutures --> F_collectResults[Call result on each future to build s_params_list]
F_collectResults --> G_postProcess[Compute wavelengths, drop, through, error metrics]
G_postProcess --> H_plot[Plot coupler responses and fitting errors]
H_plot --> I_continueNotebook[Continue with filter modeling and full system simulation]
I_continueNotebook --> J_end[End notebook]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The notebook metadata now hardcodes a local kernel name (
"display_name": "gplugins (3.11.14)"and Python 3.11.14); consider reverting those fields so the notebook remains environment-agnostic for other users. - In
write_sparameters, theweb.runcall now includes a TODO aboutComponentModelernot being supported; it may be better to either implement a minimal conversion or explicitly raise a clear error when aComponentModelerinstance is passed instead of relying on a comment that can be overlooked.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The notebook metadata now hardcodes a local kernel name (`"display_name": "gplugins (3.11.14)"` and Python 3.11.14); consider reverting those fields so the notebook remains environment-agnostic for other users.
- In `write_sparameters`, the `web.run` call now includes a TODO about `ComponentModeler` not being supported; it may be better to either implement a minimal conversion or explicitly raise a clear error when a `ComponentModeler` instance is passed instead of relying on a comment that can be overlooked.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
The end of this notebook is commented-out as meant to have a bug. I took a look at it, and in the process founds more errors.
Currently, the Notebook runs successfully IF AND ONLY IF the test data is downloaded trough
make test-data. Let me know if this is enough to merge.Changelog
gdsfactoryto be able to rungf.gpdk.PDK.activate()without error. 97a5d69Future work
docs/folder and veryfy that the documentation builds.tidy3d.wed.run()has been changed from the version the pluggins expects. In version2.8.5(current requirements is"tidy3d>=2.8.2,<2.9"), it expects a mandatory task name. However, it seems that in newer versions this is no longer the case.ComponentModelerclass.In general, I think that the
tidy3dpluggin might need some work that goes beyon just fixing this Notebook.Summary by Sourcery
Fix the cascaded MZI workflow notebook to run end-to-end and align the tidy3d plugin with updated API requirements.
Bug Fixes:
Enhancements:
Build: