Skip to content

Conversation

@momchil-flex
Copy link
Collaborator

@momchil-flex momchil-flex commented Jan 29, 2026

The tensorial mode solver in tidy3d-extras has been made more accurate. Notably, for nominally lossless, fully tensorial media, the mode solver should return very close to lossless modes.

To avoid confusion, we are removing the legacy local tensorial mode solver. Local runs are still fully supported (and more accurate!) they just require installing tidy3d-extras.


Note

Medium Risk
Changes affect the mode-solver execution path and error behavior for tensorial/angled/aniso cases, and rely on optional tidy3d-extras availability; diagonal/local and server paths should remain unchanged but need regression coverage.

Overview
Local mode solving now selects the best available solver at runtime: ModeSolver/ModeSimulation.run_local() will use tidy3d-extras’ mode solver when present, otherwise fall back to the base solver.

The legacy in-core fully tensorial eigenmode implementation is removed; attempting a tensorial solve (non-zero ModeSpec.angle_theta or fully anisotropic media) without tidy3d-extras now raises a clear NotImplementedError instructing users to install tidy3d[extras] or run via the server.

Tests and docs are updated accordingly: new coverage asserts the error/fallback behavior and skips tensorial-local tests when extras aren’t available, and plugin tests that directly exercised compute_modes/angled tensorial paths are removed.

Written by Cursor Bugbot for commit 3cb8539. This will update automatically on new commits. Configure here.

@momchil-flex momchil-flex force-pushed the momchil/anisotropic_mode branch from d657e01 to a266ac8 Compare January 29, 2026 12:00
@momchil-flex momchil-flex marked this pull request as ready for review January 30, 2026 10:59
Copilot AI review requested due to automatic review settings January 30, 2026 10:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR transitions the fully tensorial mode solver from local implementation to requiring either tidy3d-extras or server execution. The change aims to consolidate on a more accurate tensorial mode solver implementation available in tidy3d-extras while maintaining backward compatibility for diagonal (non-tensorial) mode solving.

Changes:

  • Replaces the local tensorial mode solver implementation with a NotImplementedError that provides clear instructions to users
  • Adds dynamic solver selection via _get_solver_func() that prefers tidy3d-extras when available
  • Includes comprehensive test coverage for both with/without tidy3d-extras scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
tidy3d/components/mode/solver.py Removed ~130 lines of tensorial solver implementation; replaced with error message directing users to tidy3d-extras or server
tidy3d/components/mode/mode_solver.py Added _get_solver_func() to dynamically select between extras and base solver; updated solver calls to use this function
tests/test_components/test_mode.py Added comprehensive tests for tensorial solver error handling, tidy3d-extras integration, and diagonal solver functionality
CHANGELOG.md Documented the breaking change with clear user guidance

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

- Add explanatory comment to except clause in _get_solver_func
- Fix import style in test (use 'from' import)
- Disable local subpixel in test to prevent tidy3d-extras bypass
@momchil-flex momchil-flex force-pushed the momchil/anisotropic_mode branch from a266ac8 to 3cb8539 Compare February 2, 2026 13:38
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2026

Diff Coverage

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

  • tidy3d/components/mode/mode_solver.py (75.0%): Missing lines 126,131-132,134
  • tidy3d/components/mode/solver.py (100%)

Summary

  • Total: 18 lines
  • Missing: 4 lines
  • Coverage: 77%

tidy3d/components/mode/mode_solver.py

Lines 122-138

  122     ImportError
  123         If the local solver could not be imported (e.g., scipy not installed).
  124     """
  125     if not LOCAL_SOLVER_IMPORTED:
! 126         raise ImportError(IMPORT_ERROR_MSG)
  127 
  128     # Try to get tidy3d-extras solver (handles all cases including tensorial)
  129     try:
  130         _check_tidy3d_extras_available(quiet=True)
! 131         if tidy3d_extras["mod"] is not None:
! 132             from tidy3d_extras.mode import EigSolver as ExtrasEigSolver
  133 
! 134             return ExtrasEigSolver.compute_modes
  135     except (Tidy3dImportError, ImportError, AttributeError):
  136         # tidy3d-extras is optional; if unavailable or incompatible, fall back to base solver.
  137         pass


if not LOCAL_SOLVER_IMPORTED:
raise ImportError(IMPORT_ERROR_MSG)
solver_func = _get_solver_func()
Copy link
Contributor

@caseyflex caseyflex Feb 5, 2026

Choose a reason for hiding this comment

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

Why not use the @supports_local_subpixel decorator and related logic like in _solve_all_freqs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that handling is somewhat more flexible actually, in addition to already being factored out into a subroutine. It lets the user control the behavior: config.use_local_subpixel=None means use extras if available, False means don't use it, True means always use it. I would do the same thing here, and just raise an error if tensorial mode solve is required and tidy3d_extras["use_local_subpixel"] is False.


# Also disable local subpixel to prevent tidy3d-extras from bypassing our patched code path
# when tidy3d-extras is installed and licensed
monkeypatch.setitem(tidy3d_extras, "use_local_subpixel", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

on a related note, not sure about this test logic. Seems we want the error when this setting is None too.

Copy link
Contributor

Choose a reason for hiding this comment

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

To take advantage of the integration tests, in addition to the mock test for the error message, you could also have a test that checks whether tidy3d_extras is available (I think there are others like that) and has different behavior in the two cases

assert result_aniso.modes_raw.n_eff is not None


def test_diagonal_mode_solver_still_works():
Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems redundant



@td.packaging.disable_local_subpixel
def test_mode_solver_straight_vs_angled():
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, so this test previously had local subpixel disabled, presumably because subpixel would change the results of the test. I guess your new tests cover this use case? We could have one of them in the frontend but only run if tidy3d_extras is present?

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.

2 participants