Skip to content

Conversation

@caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Oct 31, 2025

This PR:

  • Moves nonlinear models to a separate file, to clean up medium.py a bit
  • Cleans up a few functions that are no longer needed after use_complex_fields was removed
  • Changes Simulation.subsection treatment of sources in the presence of nonlinear materials

Greptile Overview

Updated On: 2025-10-31 17:39:54 UTC

Greptile Summary

Successfully refactored nonlinear models into a separate nonlinear.py file to improve code organization and reduce the size of medium.py.

Key changes:

  • Created new tidy3d/components/nonlinear.py with all nonlinear model classes (NonlinearModel, NonlinearSusceptibility, TwoPhotonAbsorption, KerrNonlinearity, NonlinearSpec)
  • Removed helper methods (_get_n0, _get_freq0, _hardcode_medium_freqs) that were previously used for frequency hardcoding
  • Updated Simulation.subsection to preserve sources instead of hardcoding frequency info into nonlinear materials
  • Updated all imports and exports to reference the new module location
  • Updated tests to reflect the removal of frequency hardcoding behavior

The refactoring is clean and maintains backward compatibility for public APIs.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The refactoring is straightforward code organization without functional changes to the nonlinear models themselves. All imports/exports are properly updated, tests are adjusted correctly, and the removed helper methods are no longer referenced anywhere in the codebase.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/nonlinear.py 5/5 New file created to separate nonlinear models from medium.py. Code moved cleanly with proper imports and structure.
tidy3d/components/medium.py 5/5 Removed nonlinear class definitions and helper methods (_get_n0, _get_freq0, _hardcode_medium_freqs). Added imports from new nonlinear module.
tidy3d/components/simulation.py 5/5 Removed subsection logic that hardcoded source frequency into nonlinear models, now preserves sources in subsections instead.

Sequence Diagram

sequenceDiagram
    participant User
    participant Simulation
    participant Medium
    participant NonlinearSpec
    participant NonlinearModel
    
    Note over Medium,NonlinearModel: Before: All in medium.py
    Note over Medium,NonlinearModel: After: Separated into nonlinear.py
    
    User->>Medium: Create medium with nonlinear_spec
    Medium->>NonlinearSpec: Import from nonlinear module
    NonlinearSpec->>NonlinearModel: Contains models list
    
    User->>Simulation: Call subsection()
    Note over Simulation: Removed frequency hardcoding
    Simulation->>Simulation: Preserve sources in subsection
    Simulation-->>User: Return subsection with sources
Loading

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.

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the chore/casey/movenonlinear branch from 28931ca to 51ea669 Compare October 31, 2025 00:08
@caseyflex
Copy link
Contributor Author

@greptile

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.

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@yaugenst-flex yaugenst-flex changed the title chore: Move nonlinear models to separate file chore: Move nonlinear models to separate file (FXC-3925) Oct 31, 2025
@caseyflex caseyflex force-pushed the chore/casey/movenonlinear branch from 51ea669 to 7cba2aa Compare October 31, 2025 16:58
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

great to see the 8k medium.py file finally get chopped into pieces. I think the new logic for setting wavelength into auto grid is a more robust behavior, I guess just need to skip it if there are no sources to begin with to prevent unintentional errors. Also, could you add a note in the subsection's docstring that auto grid will be assigned a wavelength if not yet provided because automatic removing of sources

@caseyflex
Copy link
Contributor Author

great to see the 8k medium.py file finally get chopped into pieces. I think the new logic for setting wavelength into auto grid is a more robust behavior, I guess just need to skip it if there are no sources to begin with to prevent unintentional errors. Also, could you add a note in the subsection's docstring that auto grid will be assigned a wavelength if not yet provided because automatic removing of sources

Thanks. I actually realized I don't need to hardcode the wavelength in the final simulation subsection -- I just need to hardcode it for intermediate steps where the sources are not included to avoid validation. So maybe I'll leave the behavior as it was previously with regards to grid...

@caseyflex caseyflex force-pushed the chore/casey/movenonlinear branch from 7cba2aa to 33379ad Compare October 31, 2025 17:09
@caseyflex
Copy link
Contributor Author

@greptile

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/components/medium.py, line 105-106 (link)

    logic: Duplicate constant definitions - these constants are now defined in both medium.py and nonlinear.py, but are only used in nonlinear.py. Remove from medium.py.

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the chore/casey/movenonlinear branch 2 times, most recently from c899d32 to 7226fb9 Compare October 31, 2025 17:34
@caseyflex
Copy link
Contributor Author

OK, I was actually pretty confused about this. I changed it as follows, hopefully it makes sense now:

  • The only change to simulation subsection is that source frequencies are no longer hardcoded for nonlinear materials. That is because the handling of central frequency has changed, and will be robust for mode solving with nonlinear materials.
  • There was no sense in hardcoding the wavelength based on the new sources, as those should be included anyway
  • It was messy to handle sources separately when nonlinear materials are present, and there was no need anyway.

@caseyflex
Copy link
Contributor Author

@greptile

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.

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

Diff Coverage

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

  • tidy3d/init.py (100%)
  • tidy3d/components/medium.py (100%)
  • tidy3d/components/nonlinear.py (93.7%): Missing lines 33,38,43,57,361,394

Summary

  • Total: 97 lines
  • Missing: 6 lines
  • Coverage: 93%

tidy3d/components/nonlinear.py

Lines 29-47

  29         """Check that the model is compatible with the medium."""
  30         from .medium import AbstractCustomMedium, DispersiveMedium, Medium
  31 
  32         if isinstance(medium, AbstractCustomMedium):
! 33             raise ValidationError(
  34                 f"'NonlinearModel' of class '{type(self).__name__}' is not currently supported "
  35                 f"for medium class '{type(medium).__name__}'."
  36             )
  37         if medium.is_time_modulated:
! 38             raise ValidationError(
  39                 f"'NonlinearModel' of class '{type(self).__name__}' is not currently supported "
  40                 f"for time-modulated medium class '{type(medium).__name__}'."
  41             )
  42         if not isinstance(medium, (Medium, DispersiveMedium)):
! 43             raise ValidationError(
  44                 f"'NonlinearModel' of class '{type(self).__name__}' is not currently supported "
  45                 f"for medium class '{type(medium).__name__}'."
  46             )

Lines 53-61

  53 
  54     @property
  55     def complex_fields(self) -> bool:
  56         """Whether the model uses complex fields."""
! 57         return False
  58 
  59     @property
  60     def aux_fields(self) -> list[str]:
  61         """List of available aux fields in this model."""

Lines 357-365

  357     @pd.validator("models", always=True)
  358     def _no_duplicate_models(cls, val):
  359         """Ensure each type of model appears at most once."""
  360         if val is None:
! 361             return val
  362         models = [model.__class__ for model in val]
  363         models_unique = set(models)
  364         if len(models) != len(models_unique):
  365             raise ValidationError(

Lines 390-398

  390     @pd.validator("models", always=True)
  391     def _consistent_models(cls, val):
  392         """Ensure that parameters shared between models are consistent."""
  393         if val is None:
! 394             return val
  395         n0 = None
  396         for model in val:
  397             if isinstance(model, (KerrNonlinearity, TwoPhotonAbsorption)):
  398                 if model.n0 is not None:

@yaugenst-flex
Copy link
Collaborator

yaugenst-flex commented Oct 31, 2025

great to see the 8k medium.py file finally get chopped into pieces.

@dbochkov-flexcompute only 7k more lines to go 😂

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Thanks @caseyflex, this is great, only have a couple of minor nits.

@caseyflex caseyflex force-pushed the chore/casey/movenonlinear branch 2 times, most recently from edbf4b1 to 0a2424b Compare October 31, 2025 22:29
@caseyflex caseyflex enabled auto-merge October 31, 2025 22:30
@caseyflex caseyflex force-pushed the chore/casey/movenonlinear branch from 0a2424b to 6f9cbbd Compare October 31, 2025 22:57
@caseyflex caseyflex added this pull request to the merge queue Oct 31, 2025
Merged via the queue into develop with commit 80e660c Nov 1, 2025
26 checks passed
@caseyflex caseyflex deleted the chore/casey/movenonlinear branch November 1, 2025 00:07
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.

4 participants