FXC-4643: update optionality and default values in ModeSortSpec

[momchil-flex]

https://flow360.atlassian.net/browse/FXC-4643

I realized that the best default values for sort_order are kind of key-dependent, so went beyond the description in the Jira ticket and made them explicitly so. The defaults are automatically put in the description of the field so it's clear for the user.

---

Note

Introduces key-aware defaulting and minor performance improvements around mode sorting.

• ModeSortSpec defaults: sort_key now required with default "n_eff"; sort_order becomes optional and is derived via validator from sort_key and sort_reference using new MODE_DATA_KEY_SORT_ORDER map; dynamic field description generated.
• Validation & compatibility: Updated filter_pol exclusivity check to allow default ModeSortSpec but error on any customized sorting/filtering.
• Sorting performance: Added fast-path in ModeSolverData.overlap_sort to skip re-sorting when already ordered; always precompute sort_metric.
• Schema changes: JSON schemas updated to reflect new defaults (sort_key="n_eff", sort_order default removed/behavioral); applied across Simulation/Mode/EME/TerminalComponentModeler.
• Tests & docs: New tests for defaulting behavior and filter_pol interactions; CHANGELOG entry updated.

^{Written by Cursor Bugbot for commit d42ee4f. This will update automatically on new commits. Configure here.}

Greptile Overview

Greptile Overview

Greptile Summary

Overview

This PR refactors ModeSortSpec to introduce key-aware default values for mode sorting. The main changes are:

1. sort_key is now required with a default value of n_eff (previously optional with None default)
2. sort_order is now optional with a default of None, which is automatically resolved based on sort_key and sort_reference via a new validator
3. New dictionary MODE_DATA_KEY_SORT_ORDER maps each mode data key to its natural sort order (e.g., n_eff maps to descending, k_eff maps to ascending)
4. Updated validator for filter_pol compatibility checks to account for the new default values
5. Comprehensive tests covering the new defaulting behavior
6. Schema updates across 4 JSON files to reflect the new defaults

Design Intent

The PR addresses the observation that optimal default sort orders are key-dependent. For example, higher n_eff values (more confined modes) should come first (descending), while lower k_eff values (less lossy modes) should come first (ascending). The new design makes this explicit and automatic.

Critical Issues Found

1. Breaking Test in test_mode_solver.py

The validator logic change will cause test_mode_spec_filter_pol_sort_spec_exclusive in tests/test_plugins/test_mode_solver.py at line 1327 to fail. The test expects a ValidationError when filter_pol is used with an explicit sort_key equal to n_eff, but this will now pass validation because it produces an identical object to the default ModeSortSpec.

2. Inconsistency with monitor_data.py

The code in tidy3d/components/data/monitor_data.py at line 2384 checks if sort_spec.sort_key is not None, which will now always be True since sort_key is no longer optional. This makes the else branch at line 2410 unreachable dead code. While this may be intentional (always apply sorting), the implementation should be updated or documented to reflect this change.

Positive Aspects

• Well-structured implementation with clear separation of concerns
• Comprehensive test coverage for the new functionality
• Module-level validation ensures MODE_DATA_KEY_SORT_ORDER stays synchronized with MODE_DATA_KEYS
• Dynamic description generation makes the field documentation maintainable
• CHANGELOG entry is clear and follows project conventions
• Schema files are consistently updated across all simulation types

Confidence Score: 3/5

• This PR has critical issues that will cause test failures and introduces inconsistencies with existing code
• Score of 3 reflects two critical issues: (1) a breaking test that will fail due to the validator logic change, and (2) inconsistency with monitor_data.py that checks for None on a field that can no longer be None. While the core design is sound and the implementation is well-tested, these issues must be addressed before merging to avoid breaking the build and creating dead code.
• Pay close attention to tidy3d/components/mode_spec.py (validator logic) and ensure tests/test_plugins/test_mode_solver.py is updated to reflect the new behavior. Also verify that tidy3d/components/data/monitor_data.py doesn't have logic that depends on sort_key being None.

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/components/mode_spec.py	3/5	Adds key-aware sort_order defaulting and makes sort_key required. Contains logic issues with validator that breaks existing test and creates inconsistency with monitor_data.py.
tests/test_components/test_mode.py	4/5	Adds comprehensive tests for default sort_order behavior and filter_pol compatibility. Tests are well-structured and cover the new functionality thoroughly.
CHANGELOG.md	5/5	Clear, descriptive changelog entry explaining the optionality changes and new default behavior. Follows project conventions.
schemas/Simulation.json	5/5	Schema correctly updated to reflect new defaults: sort_key defaults to n_eff, sort_order no longer has a default (computed by validator).

Sequence Diagram

sequenceDiagram
    participant User
    participant ModeSortSpec
    participant Validator
    participant MODE_DATA_KEY_SORT_ORDER

    User->>ModeSortSpec: Create ModeSortSpec(sort_key, sort_reference, sort_order)
    ModeSortSpec->>ModeSortSpec: Set sort_key (default: "n_eff")
    ModeSortSpec->>ModeSortSpec: Set sort_reference (default: None)
    ModeSortSpec->>ModeSortSpec: Set sort_order (default: None)
    
    ModeSortSpec->>Validator: Run _set_default_sort_order validator
    
    alt sort_order is not None
        Validator->>ModeSortSpec: Return explicit sort_order
    else sort_reference is not None
        Validator->>ModeSortSpec: Return "ascending" (closest to reference)
    else sort_reference is None
        Validator->>MODE_DATA_KEY_SORT_ORDER: Get default for sort_key
        MODE_DATA_KEY_SORT_ORDER->>Validator: Return "descending" or "ascending"
        Validator->>ModeSortSpec: Return key-specific default
    end
    
    ModeSortSpec->>User: Return configured ModeSortSpec

Loading

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/components/mode_spec.py	3/5	Adds key-aware sort_order defaulting and makes sort_key required. Contains logic issues with validator that breaks existing test and creates inconsistency with monitor_data.py.
tests/test_components/test_mode.py	4/5	Adds comprehensive tests for default sort_order behavior and filter_pol compatibility. Tests are well-structured and cover the new functionality thoroughly.
CHANGELOG.md	5/5	Clear, descriptive changelog entry explaining the optionality changes and new default behavior. Follows project conventions.
schemas/Simulation.json	5/5	Schema correctly updated to reflect new defaults: sort_key defaults to n_eff, sort_order no longer has a default (computed by validator).

Sequence Diagram

sequenceDiagram
    participant User
    participant ModeSortSpec
    participant Validator
    participant MODE_DATA_KEY_SORT_ORDER

    User->>ModeSortSpec: Create ModeSortSpec(sort_key, sort_reference, sort_order)
    ModeSortSpec->>ModeSortSpec: Set sort_key (default: "n_eff")
    ModeSortSpec->>ModeSortSpec: Set sort_reference (default: None)
    ModeSortSpec->>ModeSortSpec: Set sort_order (default: None)
    
    ModeSortSpec->>Validator: Run _set_default_sort_order validator
    
    alt sort_order is not None
        Validator->>ModeSortSpec: Return explicit sort_order
    else sort_reference is not None
        Validator->>ModeSortSpec: Return "ascending" (closest to reference)
    else sort_reference is None
        Validator->>MODE_DATA_KEY_SORT_ORDER: Get default for sort_key
        MODE_DATA_KEY_SORT_ORDER->>Validator: Return "descending" or "ascending"
        Validator->>ModeSortSpec: Return key-specific default
    end
    
    ModeSortSpec->>User: Return configured ModeSortSpec

Loading

[Copilot]

Pull request overview

This PR updates the ModeSortSpec class to make sort_key required with a default value of "n_eff" and sort_order optional with intelligent defaults based on context. The change introduces key-dependent default sort orders, where each mode data key has a natural ordering (descending for confinement-related metrics, ascending for loss/area metrics), and reference-based sorting always defaults to ascending (closest values first).

Key changes:

• Made sort_key required with default "n_eff" (previously optional)
• Made sort_order optional with context-aware defaults via validator
• Added MODE_DATA_KEY_SORT_ORDER dictionary mapping each key to its natural sort order

Reviewed changes

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

Show a summary per file

File	Description
tidy3d/components/mode_spec.py	Introduces MODE_DATA_KEY_SORT_ORDER mapping, adds validator for context-aware sort_order defaults, updates field definitions and validation logic
tests/test_components/test_mode.py	Adds comprehensive tests for new default sort order behavior and interaction with deprecated filter_pol
schemas/TerminalComponentModeler.json	Updates JSON schema to reflect new defaults: sort_key: "n_eff", sort_order: "descending"
schemas/Simulation.json	Updates JSON schema to reflect new defaults: sort_key: "n_eff", sort_order: "descending"
schemas/ModeSimulation.json	Updates JSON schema to reflect new defaults: sort_key: "n_eff", sort_order: "descending"
schemas/EMESimulation.json	Updates JSON schema to reflect new defaults: sort_key: "n_eff", sort_order: "descending"
docs/notebooks	Updates subproject commit reference
docs/faq	Updates subproject commit reference
CHANGELOG.md	Documents the breaking change in ModeSortSpec field defaults and behavior

---

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

[greptile-apps]

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

tidy3d/components/mode_spec.py
The updated validator logic will break an existing test in tests/test_plugins/test_mode_solver.py at line 1327.

Previously, creating a ModeSortSpec with explicit sort_key equal to the default was considered "custom" because the field was explicitly set. Now, both the default ModeSortSpec and one created with sort_key equal to n_eff result in identical objects, so the validator comparison finds them equal.

The test test_mode_spec_filter_pol_sort_spec_exclusive expects a ValidationError when filter_pol is used with sort_key set to n_eff, but this will now pass validation because the comparison finds them identical to the default.

You need to either update the test to use a genuinely different sort_key like k_eff, or adjust the validator logic to detect when fields were explicitly provided.

Prompt To Fix With AI

This is a comment left during a code review.
Path: tidy3d/components/mode_spec.py
Line: 701:718

Comment:
The updated validator logic will break an existing test in tests/test_plugins/test_mode_solver.py at line 1327.

Previously, creating a ModeSortSpec with explicit sort_key equal to the default was considered "custom" because the field was explicitly set. Now, both the default ModeSortSpec and one created with sort_key equal to n_eff result in identical objects, so the validator comparison finds them equal.

The test test_mode_spec_filter_pol_sort_spec_exclusive expects a ValidationError when filter_pol is used with sort_key set to n_eff, but this will now pass validation because the comparison finds them identical to the default.

You need to either update the test to use a genuinely different sort_key like k_eff, or adjust the validator logic to detect when fields were explicitly provided.

How can I resolve this? If you propose a fix, please make it concise.

[momchil-flex]

@greptile-ai what do you think now?

[greptile-apps]

Let me search for the specific code around line 2384 mentioned in the issue: