-
Notifications
You must be signed in to change notification settings - Fork 49
EMode plugin #591
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
base: main
Are you sure you want to change the base?
EMode plugin #591
Conversation
Reviewer's GuideIntroduces a new EMode plugin by updating documentation, configuration, project metadata, and adding both example notebook and plugin code with tests for seamless integration of the EMode mode solver and EME propagation into GDSFactory. Sequence Diagram: EMode Plugin Call ForwardingsequenceDiagram
actor UserCode
participant gplugins_emode as "gplugins.emode"
participant emc_lib as "emodeconnection"
UserCode->>gplugins_emode: call function (e.g., EMode(), open_file())
activate gplugins_emode
gplugins_emode->>emc_lib: call corresponding function (e.g., EMode(), open_file())
activate emc_lib
emc_lib-->>gplugins_emode: return result
deactivate emc_lib
gplugins_emode-->>UserCode: return result
deactivate gplugins_emode
Class Diagram: gplugins.emode Module Structure and DependenciesclassDiagram
class GPluginsEMode {
<<Python Module>>
name: "gplugins.emode"
+EMode
+open_file()
+get()
+inspect()
}
class EmodeconnectionLib {
<<Python Library>>
name: "emodeconnection"
+EMode
+open_file()
+get()
+inspect()
}
GPluginsEMode ..> EmodeconnectionLib : imports and re-exports symbols
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @estanton22 - I've reviewed your changes - here's some feedback:
- Rename the test file to start with
test_(e.g.,test_SOI.py) and add assertions so pytest will discover and validate the plugin functionality automatically. - Add a runtime check or clear error message when the external EMode software isn't installed or accessible, to guide users through missing dependencies.
- Expand the new notebook with end-to-end example cells that execute a mode solve and visualize results, so users see the plugin in action.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - ``tidy3d``: Finite difference Frequency Domain (FDFD). | ||
| - ``MPB``: FDFD with periodic boundary conditions. | ||
| - ``Femwell``: Finite Element (FEM). | ||
| - ``meow``: The tidy3d mode solver is also used by the MEOW plugin to get the Sparameters of components via Eigenmode Expansion. |
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.
issue (typo): Typo: "Sparameters" should likely be "S-parameters".
"S-parameters" is the correct term for scattering parameters in this context.
| - ``meow``: The tidy3d mode solver is also used by the MEOW plugin to get the Sparameters of components via Eigenmode Expansion. | |
| - ``meow``: The tidy3d mode solver is also used by the MEOW plugin to get the S-parameters of components via Eigenmode Expansion. |
joamatab
left a comment
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.
Looks good, i would improve the tests if possible
gplugins/emode/tests/SOI.py
Outdated
| em.plot() | ||
|
|
||
| ## Close EMode | ||
| em.close() No newline at end of file |
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.
it would be great to have some actual tests
for example:
def test_neff():
assert np.isclose(neff, 2.4)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.
Thanks Joaquin. I just removed this test because it was actually more of an example. Once I build some real integrations with GDSFactory, I will add some real tests.
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.
sounds good, I'll keep the PR open then
once you can directly make mode simulations from GDSFactory layerstack we can merge this
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.
Hi Joaquin, I added some functionality for EMode to use the LayerStack from GDSFactory.
The test (gplugins/emode/tests/SOI.py) works, but I'd like some feedback on how I implemented the CrossSection specifications.
If it looks good to you, I think it's ready to merge.
estanton22
left a comment
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.
This is an initial implementation of the EMode plugin with one test case showcasing integration with the gdsfactory LayerStack.
|
sorry for the late reply, the notebook added in this PR seem to fail |
890de07 to
368bb56
Compare
|
I fixed the issue with compiling the emode notebook. I also tested the build docs run commands and they completed without errors on my machine. |
I added a very basic EMode plugin and some documentation.
Summary by Sourcery
Integrate the EMode Photonix solver as a new plugin in GDSFactory, including documentation updates, example notebook, project dependency, and a basic integration test
New Features:
Enhancements:
Build:
Documentation:
Tests: