Skip to content

Conversation

@caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Nov 4, 2025

This PR allows nonlinear_spec to be applied to CustomMedium and CustomDispersiveMedium. Anisotropic media are still not supported, and the nonlinearity cannot vary spatially.

Greptile Overview

Updated On: 2025-11-05 19:15:49 UTC

Greptile Summary

This PR extends nonlinearity support to CustomMedium and CustomDispersiveMedium classes. The implementation correctly validates that anisotropic media are rejected by checking the is_isotropic property before the type check.

Key changes:

  • Reordered validation checks to check is_time_modulated first, then is_isotropic, and finally the type check
  • Added CustomMedium and CustomDispersiveMedium to the list of allowed medium types in the isinstance check
  • Updated imports to include the new custom medium types
  • Updated error messages to be more specific about what is being rejected

Issues found:

  • Missing CHANGELOG entry (per custom rule a109c62a-aa8c-4cc4-b515-0ad947c47929)

Note: The previous comment about missing anisotropic validation has been addressed - the code properly checks medium.is_isotropic before allowing custom media.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk, pending CHANGELOG entry addition
  • The implementation is correct and properly validates media constraints. The validation logic is clear: it checks time-modulation first, then isotropy, and finally the type. Both CustomMedium and CustomDispersiveMedium have the is_isotropic property, so the check will correctly reject anisotropic custom media. The only issue is a missing CHANGELOG entry, which is a documentation concern rather than a code quality issue.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/nonlinear.py 4/5 Enabled nonlinearity support for CustomMedium and CustomDispersiveMedium by reordering validation checks and updating allowed types. The validation properly checks is_isotropic before type checking.

Sequence Diagram

sequenceDiagram
    participant User
    participant NonlinearSpec
    participant NonlinearModel
    participant Medium
    
    User->>NonlinearSpec: Create NonlinearSpec with models
    User->>Medium: Create CustomMedium/CustomDispersiveMedium with nonlinear_spec
    NonlinearSpec->>NonlinearModel: Validate medium compatibility
    NonlinearModel->>Medium: Check is_time_modulated property
    alt is_time_modulated == True
        NonlinearModel-->>User: Raise ValidationError (time-modulated not supported)
    end
    NonlinearModel->>Medium: Check is_isotropic property
    alt is_isotropic == False
        NonlinearModel-->>User: Raise ValidationError (anisotropic not supported)
    end
    NonlinearModel->>Medium: Check medium type
    alt not in (Medium, DispersiveMedium, CustomMedium, CustomDispersiveMedium)
        NonlinearModel-->>User: Raise ValidationError (unsupported medium type)
    end
    NonlinearModel-->>NonlinearSpec: Validation passed
    NonlinearSpec-->>User: Medium with nonlinearity created successfully
Loading

@caseyflex caseyflex marked this pull request as ready for review November 5, 2025 19:05
@caseyflex
Copy link
Contributor Author

caseyflex commented Nov 5, 2025

@yaugenst-flex once we support anisotropic (not fully anisotropic, just diagonally anisotropic) nonlinear medium (a minor change perhaps), we can get rid of this _validate_medium_type function you don't like, in favor of the _has_incompatibilities in medium.py. But no need to change this here...

@caseyflex caseyflex changed the title Allow nonlinearity with custom medium Allow nonlinearity with custom medium (FXC-3548) Nov 5, 2025
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.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the casey/nonlinearcustom2 branch from d9ca2d4 to bee7644 Compare November 5, 2025 19:11
@caseyflex
Copy link
Contributor Author

@greptile

@caseyflex caseyflex force-pushed the casey/nonlinearcustom2 branch 3 times, most recently from f960e6c to 8d3c139 Compare November 5, 2025 19:37
@caseyflex
Copy link
Contributor Author

@yaugenst-flex @momchil-flex I also got rid of the warning that evaluating the permittivity of a nonlinear medium ignores the nonlinearity. I think this is obvious (we aren't running the simulation so there is no source intensity anyway). And this warning comes up all the time when I don't need it, like in plotting, or computing source fields for simulations with nonlinear + plane wave for example. Let me know if you think this is ok; an alternative is suppressing this warning where we don't want it, but I personally would never want to see it.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

Diff Coverage

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

  • tidy3d/components/nonlinear.py (100%)

Summary

  • Total: 4 lines
  • Missing: 0 lines
  • Coverage: 100%

@yaugenst-flex
Copy link
Collaborator

@yaugenst-flex @momchil-flex I also got rid of the warning that evaluating the permittivity of a nonlinear medium ignores the nonlinearity. I think this is obvious (we aren't running the simulation so there is no source intensity anyway). And this warning comes up all the time when I don't need it, like in plotting, or computing source fields for simulations with nonlinear + plane wave for example. Let me know if you think this is ok; an alternative is suppressing this warning where we don't want it, but I personally would never want to see it.

Hmm I'm not sure about this. I think the warning is good, even if it might be obvious considering the rest of the setup. I feel like it's an easy thing to miss and a reminder that you're getting the linear response is helpful. I think if warning noise is the issue, it might be better to perhaps demote it to log.info or logging only once per medium call, something along those lines. But dropping it entirely feels like a regression to me.

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.

Yeah not sure. I guess basically the definition of permittivity is 1 + chi(1), i.e. taking only the linear susceptibility. So indeed one shouldn't expect anything different when "getting epsilon". But of course in some cases we do think of a nonlinearity-induced change to the linear response...

I guess downgrading to INFO is fine, even though practically speaking it will never be seen by users. :)

@caseyflex caseyflex force-pushed the casey/nonlinearcustom2 branch from 8d3c139 to 8a6f27f Compare November 7, 2025 19:51
@caseyflex
Copy link
Contributor Author

Ok, I'll just restore the previous handling. If it gets annoying we can address it in a future PR.

@caseyflex caseyflex enabled auto-merge November 7, 2025 19:53
@caseyflex caseyflex added this pull request to the merge queue Nov 7, 2025
@caseyflex caseyflex removed this pull request from the merge queue due to a manual request Nov 7, 2025
@caseyflex caseyflex force-pushed the casey/nonlinearcustom2 branch from 8a6f27f to 5eb28b4 Compare November 7, 2025 21:21
@caseyflex caseyflex enabled auto-merge November 7, 2025 21:21
@caseyflex caseyflex added this pull request to the merge queue Nov 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 7, 2025
@caseyflex caseyflex force-pushed the casey/nonlinearcustom2 branch from 5eb28b4 to d41f500 Compare November 8, 2025 18:26
@caseyflex caseyflex enabled auto-merge November 8, 2025 18:26
@caseyflex caseyflex added this pull request to the merge queue Nov 8, 2025
Merged via the queue into develop with commit 12998d9 Nov 8, 2025
26 checks passed
@caseyflex caseyflex deleted the casey/nonlinearcustom2 branch November 8, 2025 19:37
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