Skip to content

Conversation

tylerflex
Copy link
Collaborator

@tylerflex tylerflex commented Oct 3, 2025

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 metrics
  • DesignRegion/TopologyDesignRegion: Abstracts parameter-to-geometry conversion for topology optimization
  • Metric/FluxMetric: Provides extensible framework for defining optimization objectives
  • InverseDesign: Main orchestration class that coordinates multiple device scenarios
  • OptimizerSpec: Configuration container for optimization hyperparameters

The new architecture enables multi-device optimization workflows where each DeviceSpec handles its own simulation construction and metric evaluation, while the InverseDesign 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, and autograd.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 and docs/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:

  • The PR description is minimal with only a Jira ticket reference and a brief question
  • Consider adding a more descriptive summary of the architectural changes and benefits

Important Files Changed

Changed Files
Filename Score Overview
tidy3d/plugins/invdes2/init.py 5/5 Clean public API definition exposing the five core classes of the new inverse design framework
tidy3d/plugins/invdes2/inverse_design.py 4/5 Main orchestration class that coordinates multi-device optimization with proper parameter handling
tidy3d/plugins/invdes2/device_spec.py 4/5 Core component encapsulating single device scenarios with simulation building and metric evaluation
tidy3d/plugins/invdes2/design_region.py 4/5 Abstract design region framework with topology optimization implementation for parameter-to-structure conversion
tidy3d/plugins/invdes2/metric.py 4/5 Extensible metric system with abstract base class and concrete FluxMetric implementation
tests/test_plugins/test_invdes2.py 4/5 Comprehensive test suite covering the full inverse design pipeline with emulated simulation runs
tidy3d/plugins/invdes2/optimizer_spec.py 4/5 Simple configuration dataclass for optimization hyperparameters
tidy3d/plugins/invdes2/README.md 2/5 Empty README file that needs documentation for the new API
docs/notebooks 5/5 Submodule update to sync tutorial notebooks with new invdes2 functionality
docs/faq 5/5 Submodule update likely containing documentation changes for the inverse design refactor

Confidence score: 4/5

  • This PR introduces significant architectural changes but follows established patterns and includes comprehensive testing
  • Score reflects well-structured code with good separation of concerns, though the empty README indicates work-in-progress status
  • Pay close attention to the comprehensive test file to ensure all functionality is properly validated

Sequence Diagram

sequenceDiagram
    participant User
    participant InverseDesign
    participant DeviceSpec
    participant DesignRegion
    participant Simulation as "td.Simulation"
    participant Web as "tidy3d.web"
    participant Metric

    User->>InverseDesign: "create InverseDesign with optimizer_spec and device_specs"
    User->>InverseDesign: "call get_objective(params)"
    
    InverseDesign->>InverseDesign: "_unflatten_params(params)"
    InverseDesign->>InverseDesign: "get_simulations(params)"
    
    loop for each device_spec
        InverseDesign->>DeviceSpec: "get_simulation(device_params)"
        
        loop for each design_region
            DeviceSpec->>DesignRegion: "to_structure(region_params)"
            DesignRegion-->>DeviceSpec: "return td.Structure"
        end
        
        DeviceSpec->>Simulation: "updated_copy(structures)"
        Simulation-->>DeviceSpec: "return updated simulation"
        DeviceSpec-->>InverseDesign: "return simulation"
    end
    
    InverseDesign->>InverseDesign: "run_simulations(sims)"
    InverseDesign->>Web: "run_async(sims)"
    Web-->>InverseDesign: "return BatchData"
    
    InverseDesign->>InverseDesign: "get_metric(batch_data)"
    
    loop for each device_spec
        InverseDesign->>DeviceSpec: "get_metric(sim_data)"
        
        loop for each metric
            DeviceSpec->>Metric: "evaluate(monitor_data)"
            Metric-->>DeviceSpec: "return scalar value"
        end
        
        DeviceSpec-->>InverseDesign: "return weighted sum"
    end
    
    InverseDesign-->>User: "return total objective value"
Loading

@tylerflex tylerflex marked this pull request as draft October 3, 2025 20:25
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  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

Edit Code Review Agent Settings | Greptile

Scalar multiplied with the metric value during aggregation.
"""

monitor_name: str
Copy link

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

Suggested change
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:
Copy link

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)

Suggested change
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)
Copy link

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

Suggested change
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]
Copy link

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
Copy link

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.

Copy link
Contributor

github-actions bot commented Oct 3, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/plugins/invdes2/init.py (100%)
  • tidy3d/plugins/invdes2/design_region.py (100%)
  • tidy3d/plugins/invdes2/device_spec.py (88.9%): Missing lines 60,83-85
  • tidy3d/plugins/invdes2/inverse_design.py (98.3%): Missing lines 47
  • tidy3d/plugins/invdes2/metric.py (100%)
  • tidy3d/plugins/invdes2/optimizer_spec.py (100%)

Summary

  • Total: 156 lines
  • Missing: 5 lines
  • Coverage: 96%

tidy3d/plugins/invdes2/device_spec.py

Lines 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.py

Lines 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant