-
Notifications
You must be signed in to change notification settings - Fork 66
Invdes Refresh [WIP] #2865
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: develop
Are you sure you want to change the base?
Invdes Refresh [WIP] #2865
Conversation
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.
Additional Comments (1)
-
tidy3d/plugins/invdes2/README.md
, line 1 (link)style: README is completely empty. Should include documentation explaining the new invdes2 API, usage examples, and migration guide from the original invdes plugin.
10 files reviewed, 6 comments
Scalar multiplied with the metric value during aggregation. | ||
""" | ||
|
||
monitor_name: str |
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.
style: Missing docstring for monitor_name
parameter in the docstring
monitor_name: str | |
"""Abstract base class for simulation-derived objective terms. | |
Attributes | |
---------- | |
monitor_name: | |
Name of the monitor to extract data from. | |
weight: | |
Scalar multiplied with the metric value during aggregation. | |
""" |
Context Used: Rule from dashboard
- Ensure all parameters in a function's signature are documented in its docstring. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/plugins/invdes2/metric.py
Line: 22:22
Comment:
**style:** Missing docstring for `monitor_name` parameter in the docstring
```suggestion
"""Abstract base class for simulation-derived objective terms.
Attributes
----------
monitor_name:
Name of the monitor to extract data from.
weight:
Scalar multiplied with the metric value during aggregation.
"""
```
**Context Used:** Rule from `dashboard` - Ensure all parameters in a function's signature are documented in its docstring. ([source](https://app.greptile.com/review/custom-context?memory=e4b0671a-18ac-4d59-b20d-69e8122c11f1))
How can I resolve this? If you propose a fix, please make it concise.
weight: float = 1.0 | ||
|
||
@abstractmethod | ||
def evaluate(self, mnt_data: td.MonitorData) -> float: |
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.
syntax: Parameter name in docstring (monitor_data
) doesn't match actual parameter name (mnt_data
)
def evaluate(self, mnt_data: td.MonitorData) -> float: | |
"Return a scalar score computed from `mnt_data`." |
Context Used: Rule from dashboard
- Keep docstrings and comments synchronized with code changes to ensure they are always accurate and n... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/plugins/invdes2/metric.py
Line: 26:26
Comment:
**syntax:** Parameter name in docstring (`monitor_data`) doesn't match actual parameter name (`mnt_data`)
```suggestion
"Return a scalar score computed from `mnt_data`."
```
**Context Used:** Rule from `dashboard` - Keep docstrings and comments synchronized with code changes to ensure they are always accurate and n... ([source](https://app.greptile.com/review/custom-context?memory=8947a127-e303-4dbf-b326-493579e5f047))
How can I resolve this? If you propose a fix, please make it concise.
value = 0.0 | ||
for metric in self.metrics: | ||
mnt_data = sim_data[metric.monitor_name] | ||
value = value + metric.weight * metric.evaluate(mnt_data) |
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.
style: consider using value += metric.weight * metric.evaluate(mnt_data)
for more idiomatic Python
value = value + metric.weight * metric.evaluate(mnt_data) | |
value += metric.weight * metric.evaluate(mnt_data) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/plugins/invdes2/device_spec.py
Line: 78:78
Comment:
**style:** consider using `value += metric.weight * metric.evaluate(mnt_data)` for more idiomatic Python
```suggestion
value += metric.weight * metric.evaluate(mnt_data)
```
How can I resolve this? If you propose a fix, please make it concise.
|
||
size: tuple[float, float, float] | ||
center: tuple[float, float, float] | ||
eps_bounds: tuple[float, float] |
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.
style: The eps_bounds
field is defined but never used in the implementation. Consider using it to clamp parameter values or remove if not needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/plugins/invdes2/design_region.py
Line: 40:40
Comment:
**style:** The `eps_bounds` field is defined but never used in the implementation. Consider using it to clamp parameter values or remove if not needed.
How can I resolve this? If you propose a fix, please make it concise.
|
||
geometry = td.Box(center=self.center, size=self.size) | ||
|
||
# TODO: add transformations |
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.
style: Remove TODO comment before finalizing the pull request.
Context Used: Rule from dashboard
- Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/plugins/invdes2/design_region.py
Line: 58:58
Comment:
**style:** Remove TODO comment before finalizing the pull request.
**Context Used:** Rule from `dashboard` - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... ([source](https://app.greptile.com/review/custom-context?memory=f6a669d8-0060-4f11-9cac-10ac7ee749ea))
How can I resolve this? If you propose a fix, please make it concise.
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/plugins/invdes2/device_spec.pyLines 56-64 56 return self.simulation.updated_copy(structures=structures)
57
58 def run_simulation(self, simulation: td.Simulation) -> web.SimulationData:
59 """Run the simulation via Tidy3D Web and return results."""
! 60 return web.run(simulation, task_name=self.name)
61
62 def get_metric(self, sim_data: web.SimulationData) -> float:
63 """Compute the weighted sum of metrics for this device. Lines 79-89 79 return value
80
81 def get_objective(self, params: list[np.ndarray]) -> float:
82 """Build, run, and score this device for the given parameters."""
! 83 sim = self.get_simulation(params)
! 84 sim_data = self.run_simulation(sim)
! 85 return self.get_metric(sim_data)
86
87 @property
88 def parameter_shape(self) -> list[int]:
89 """Return the shape of the parameters for each design region.""" tidy3d/plugins/invdes2/inverse_design.pyLines 43-51 43 return simulations
44
45 def run_simulations(self, sims: dict[str, td.Simulation]) -> web.BatchData:
46 """Execute multiple simulations concurrently via Tidy3D Web."""
! 47 return web.run_async(sims)
48
49 def get_metric(self, batch_data: Mapping[str, td.SimulationData]) -> float:
50 """Aggregate metrics across all devices."""
51 value = 0.0 |
https://flow360.atlassian.net/browse/FXC-3423
did I do this right?
Greptile Overview
Updated On: 2025-10-03 20:27:10 UTC
Summary
This PR introduces a complete refactor of the inverse design plugin, creating a new `invdes2` module that replaces the existing inverse design functionality with a cleaner, more modular architecture. The refactor introduces several key components that follow a clear separation of concerns pattern:Core Components Added:
DeviceSpec
: Encapsulates a single device optimization scenario, including base simulation, design regions, and metricsDesignRegion
/TopologyDesignRegion
: Abstracts parameter-to-geometry conversion for topology optimizationMetric
/FluxMetric
: Provides extensible framework for defining optimization objectivesInverseDesign
: Main orchestration class that coordinates multiple device scenariosOptimizerSpec
: Configuration container for optimization hyperparametersThe new architecture enables multi-device optimization workflows where each
DeviceSpec
handles its own simulation construction and metric evaluation, while theInverseDesign
class orchestrates parameter flattening/unflattening for optimizer compatibility and batch simulation execution via Tidy3D Web.Integration with Existing Codebase:
The plugin integrates seamlessly with the existing Tidy3D framework by leveraging
td.Structure.from_permittivity_array
for geometry creation,td.web.run
for simulation execution, andautograd.numpy
for differentiable metrics. The modular design allows for easy extension with additional metric types and design region implementations.The PR also updates two documentation submodules (
docs/notebooks
anddocs/faq
) to reflect the new functionality, and includes comprehensive tests that validate the parameter handling, simulation generation, and metric evaluation workflows using emulated runs.PR Description Notes:
Important Files Changed
Changed Files
Confidence score: 4/5
Sequence Diagram