📡 RF modular restructure v1 #2567
Open
Conversation
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
daquinteroflex
changed the base branch from
develop
to
yaugenst-flex/pydantic-v2
daquinteroflex
force-pushed
the
dario/rf_structure_refactor
branch
2 times, most recently
from
ebb1315 to
e1de4c7
Compare
yaugenst-flex
force-pushed
the
yaugenst-flex/pydantic-v2
branch
from
bc3751e to
8f478bd
Compare
yaugenst-flex
force-pushed
the
yaugenst-flex/pydantic-v2
branch
4 times, most recently
from
46e6015 to
0caf92f
Compare
yaugenst-flex
force-pushed
the
yaugenst-flex/pydantic-v2
branch
5 times, most recently
from
09d1e4a to
7e2c693
Compare
daquinteroflex
force-pushed
the
dario/rf_structure_refactor
branch
from
d35a9a3 to
978c5cc
Compare
daquinteroflex
force-pushed
the
dario/rf_structure_refactor
branch
from
9c34ca6 to
122bab6
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
35 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
tidy3d/plugins/microwave/__init__.py
Outdated
| AxisAlignedPathIntegral, | ||
| CurrentIntegralAxisAligned, | ||
| VoltageIntegralAxisAligned, | ||
| ) | ||
| from .rf_material_library import rf_material_library | ||
| from tidy3d.components.microwave.rf_material_library import rf_material_library | ||
| from tidy3d.microwave.lobe_measurer import LobeMeasurer |
There was a problem hiding this comment.
logic: Inconsistent module placement - LobeMeasurer is imported from tidy3d.microwave while all other components are moved to tidy3d.components.microwave. This breaks the modularization pattern.
| from .component_modelers.modal import ComponentModeler | ||
| from .ports.modal import Port | ||
| from tidy3d.components.microwave.component_modelers.modal import ComponentModeler | ||
| from tidy3d.components.microwave.ports.modal import Port | ||
|
|
||
| __all__ = ["ComponentModeler", "Port"] |
There was a problem hiding this comment.
style: Add explicit version deprecation warning using warnings.warn to notify users about future path changes
Suggested change
| __all__ = ["ComponentModeler", "Port"] | |
| import warnings | |
| warnings.warn( | |
| "Importing from tidy3d.plugins.smatrix.smatrix is deprecated and will be removed in a future version. " | |
| "Use 'from tidy3d.components.microwave.component_modelers.modal import ComponentModeler' instead.", | |
| DeprecationWarning, | |
| stacklevel=2, | |
| ) | |
| __all__ = ["ComponentModeler", "Port"] |
tidy3d/plugins/smatrix/__init__.py
Outdated
Comment on lines
7
to
17
| from tidy3d.components.microwave.component_modelers.modal import ( | ||
| AbstractComponentModeler, | ||
| ComponentModeler, | ||
| ModalPortDataArray, | ||
| ) | ||
| from tidy3d.components.microwave.component_modelers.terminal import TerminalComponentModeler | ||
| from tidy3d.components.microwave.data.terminal import PortDataArray, TerminalPortDataArray | ||
| from tidy3d.components.microwave.ports.coaxial_lumped import CoaxialLumpedPort | ||
| from tidy3d.components.microwave.ports.modal import Port | ||
| from tidy3d.components.microwave.ports.rectangular_lumped import LumpedPort | ||
| from tidy3d.components.microwave.ports.wave import WavePort |
There was a problem hiding this comment.
style: Consider grouping these imports by type (component modelers, data, ports) using separate import blocks with newlines for better readability
tidy3d/microwave/__init__.py
Outdated
Comment on lines
11
to
25
| __all__ = [ | ||
| "AxisAlignedPathIntegral", | ||
| "CurrentIntegralAxisAligned", | ||
| "CurrentIntegralTypes", | ||
| "CustomCurrentIntegral2D", | ||
| "CustomPathIntegral2D", | ||
| "CustomVoltageIntegral2D", | ||
| "ImpedanceCalculator", | ||
| "LobeMeasurer", | ||
| "RectangularAntennaArrayCalculator", | ||
| "VoltageIntegralAxisAligned", | ||
| "VoltageIntegralTypes", | ||
| "models", | ||
| "path_integrals_from_lumped_element", | ||
| "rf_material_library", |
There was a problem hiding this comment.
logic: Several symbols listed in all are not imported anywhere in this file (e.g., AxisAlignedPathIntegral, ImpedanceCalculator)
|
@dmarek-flex it'd be good if we can chat about this at some point |
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
|
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.
Initial reorganization into physics-domain specific structure what's shared between EM and what is RF specific.
Greptile Summary
Major reorganization of RF-specific functionality into a dedicated microwave module, separating it from the general EM components for better modularity and maintainability.
tidy3d/plugins/smatrixandtidy3d/plugins/microwaveto a new dedicatedtidy3d/microwavepackagetidy3d/microwave/__init__.py)