Skip to content

Conversation

@caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Oct 29, 2025

This old implementation has been deprecated since v2.8. It is time to remove support for it.

Some other changes:

  • n0 (if specified) must be real

Greptile Overview

Updated On: 2025-10-29 19:37:06 UTC

Greptile Summary

This PR removes the deprecated use_complex_fields parameter from nonlinear models (TwoPhotonAbsorption and KerrNonlinearity), completing the transition to the real-field implementation introduced in Tidy3D v2.8. The changes simplify the codebase by:

  • Changing beta and n2 fields from Complex to float types (real-valued only)
  • Changing n0 from Optional[Complex] to Optional[float]
  • Removing all complex-field validation logic and associated validators
  • Removing the warning about using deprecated complex fields
  • Updating all JSON schemas to reflect the new type constraints
  • Removing tests for the deprecated functionality
  • Adding a new _consistent_models validator to ensure n0 is consistent across models

The implementation is clean and thorough, with consistent updates across code, schemas, and tests. One minor issue was found: an error message is missing the actual value of n0 in the interpolation.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk after addressing the error message formatting issue
  • The PR cleanly removes deprecated functionality with thorough updates across all affected areas (code, schemas, tests). The implementation is straightforward and consistent. Score is 4 instead of 5 due to one syntax error in an error message and a minor style suggestion for the changelog.
  • Pay attention to tidy3d/components/medium.py:640-642 for the error message formatting fix

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/medium.py 4/5 Removed deprecated use_complex_fields field and associated validation logic from TwoPhotonAbsorption and KerrNonlinearity. Simplified type signatures to use only real numbers. Found one error message formatting issue.
tidy3d/components/simulation.py 5/5 Removed warning about deprecated use_complex_fields=True. Clean removal with no issues.
tests/test_components/test_medium.py 5/5 Removed tests for deprecated complex field functionality. Tests are properly cleaned up.
CHANGELOG.md 4/5 Added changelog entry for deprecated field removal. Entry could be improved with more specific details and proper categorization.
schemas/Simulation.json 5/5 Schema properly updated to remove use_complex_fields and change beta and n2 from complex to real number types.

Sequence Diagram

sequenceDiagram
    participant User
    participant TwoPhotonAbsorption
    participant KerrNonlinearity
    participant NonlinearSpec
    participant Medium
    participant Simulation

    Note over TwoPhotonAbsorption,KerrNonlinearity: Removed use_complex_fields field
    Note over TwoPhotonAbsorption: beta: Complex → float
    Note over KerrNonlinearity: n2: Complex → float
    Note over TwoPhotonAbsorption: n0: Optional[Complex] → Optional[float]
    Note over KerrNonlinearity: n0: Optional[Complex] → Optional[float]

    User->>TwoPhotonAbsorption: Create with beta (real only)
    TwoPhotonAbsorption->>TwoPhotonAbsorption: Remove _validate_beta_real
    TwoPhotonAbsorption->>TwoPhotonAbsorption: Simplify _validate_medium
    TwoPhotonAbsorption-->>User: Instance created

    User->>KerrNonlinearity: Create with n2 (real only)
    KerrNonlinearity->>KerrNonlinearity: Remove _validate_n2_real
    KerrNonlinearity->>KerrNonlinearity: Remove _validate_medium_freqs
    KerrNonlinearity-->>User: Instance created

    User->>NonlinearSpec: Create with models
    NonlinearSpec->>NonlinearSpec: Remove _consistent_old_complex_fields
    NonlinearSpec->>NonlinearSpec: Add _consistent_models (validates n0)
    NonlinearSpec-->>User: Spec created

    User->>Medium: Create with nonlinear_spec
    Medium->>NonlinearSpec: Validate models
    Medium-->>User: Medium created

    User->>Simulation: Create with nonlinear medium
    Simulation->>Simulation: Remove use_complex_fields warning
    Simulation-->>User: Simulation ready
Loading

@caseyflex caseyflex force-pushed the chore/casey/nocomplexfields branch from 1d48843 to c487be8 Compare October 29, 2025 19:28
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.

20 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the chore/casey/nocomplexfields branch from c487be8 to 4e0546b Compare October 29, 2025 19:32
@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.

20 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2025

Diff Coverage

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

  • tidy3d/components/medium.py (94.7%): Missing lines 634

Summary

  • Total: 19 lines
  • Missing: 1 line
  • Coverage: 94%

tidy3d/components/medium.py

Lines 630-638

  630     @pd.validator("models", always=True)
  631     def _consistent_models(cls, val):
  632         """Ensure that parameters shared between models are consistent."""
  633         if val is None:
! 634             return val
  635         n0 = None
  636         for model in val:
  637             if isinstance(model, (KerrNonlinearity, TwoPhotonAbsorption)):
  638                 if model.n0 is not None:

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Nice cleanup. @yaugenst this is breaking compatibility of course but I suggested going through with it because we've had deprecation warnings for a while, and it's both not correct to use complex fields and making planned expansions harder.

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 LGTM. Agree with @momchil-flex it's fine here especially since it has been deprecated for a while. My only minor nit is that it'd be better to be more specific about when something is going to be removed, but that's not for this PR to sort out 😬

@caseyflex caseyflex force-pushed the chore/casey/nocomplexfields branch from 4e0546b to 07e462f Compare October 30, 2025 18:49
@caseyflex caseyflex force-pushed the chore/casey/nocomplexfields branch from 07e462f to ec12186 Compare October 30, 2025 18:50
@caseyflex caseyflex enabled auto-merge October 30, 2025 18:50
@caseyflex caseyflex added this pull request to the merge queue Oct 30, 2025
Merged via the queue into develop with commit be4e146 Oct 30, 2025
26 checks passed
@caseyflex caseyflex deleted the chore/casey/nocomplexfields branch October 30, 2025 20:38
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