Skip to content

Conversation

@marc-flex
Copy link
Contributor

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

Note

Low Risk
Adds logging-only validation warnings and a small geometry traversal tweak; no simulation behavior changes beyond additional warnings.

Overview
Adds validation-time warnings in HeatChargeSimulation when any Cylinder in structures has an effective radius below the OpenCASCADE/gmsh meshing tolerance, including checks for tapered cylinders (radius_bottom/radius_top) and nested/translated cylinders via traverse_geometries.

Extends traverse_geometries to recurse into base.Transformed geometries, and adds tests asserting the new warnings for tiny-radius cylinders and steeply-tapered cylinders that produce too-small/negative top radii.

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

Greptile Overview

Greptile Summary

This PR adds validation-time warnings for HeatChargeSimulation when Cylinder geometries have radii below the minimum threshold required for gmsh/OpenCASCADE meshing. The implementation correctly traverses nested geometries and checks both tapered and non-tapered cylinders.

Key Changes:

  • Added MIN_GMSH_RADIUS constant (1e-6) to define OpenCASCADE tolerance
  • Implemented _warn_small_cylinder_radius validator that checks radius_bottom and radius_top against computed minimum
  • Added comprehensive test coverage for both tiny-radius and steeply-tapered cylinder cases

Issues Found:

  • Line 379 uses direct float equality (!=) which can be unreliable; should use tolerance-based comparison with np.isclose
  • Missing CHANGELOG.md entry (required per custom rules for user-facing changes)

Confidence Score: 3/5

  • Safe to merge after fixing the floating-point comparison issue and adding CHANGELOG entry
  • The feature is well-tested and adds useful warnings without changing behavior. The main concern is the floating-point comparison bug on line 379 which could cause incorrect is_tapered detection in edge cases. Missing CHANGELOG is a process violation but doesn't affect code quality.
  • Pay attention to tidy3d/components/tcad/simulation/heat_charge.py line 379 for the float comparison fix

Important Files Changed

Filename Overview
tidy3d/components/tcad/simulation/heat_charge.py Adds validator to warn about small cylinder radii; has floating-point comparison issue on line 379 that needs fixing
tests/test_components/test_heat_charge.py Adds test coverage for small cylinder radius warnings; covers both tiny radius and tapered cylinder cases

Sequence Diagram

sequenceDiagram
    participant User
    participant HeatChargeSimulation
    participant Validator as _warn_small_cylinder_radius
    participant GeomUtils as traverse_geometries
    participant Cylinder
    participant Logger as log.warning

    User->>HeatChargeSimulation: Create simulation with structures
    HeatChargeSimulation->>Validator: Validate structures field
    
    loop For each structure
        Validator->>GeomUtils: traverse_geometries(structure.geometry)
        GeomUtils-->>Validator: Yields nested geometries
        
        loop For each geometry
            alt geometry is Cylinder
                Validator->>Cylinder: Get radius_bottom
                Cylinder-->>Validator: r_bottom
                Validator->>Cylinder: Get radius_top
                Cylinder-->>Validator: r_top
                
                Validator->>Validator: Check if tapered (r_bottom != r_top)
                Validator->>Validator: Compute min_radius = max(MIN_GMSH_RADIUS, 0.01*max(abs(r_bottom), abs(r_top)))
                
                alt is_tapered
                    alt r_bottom < min_radius
                        Validator->>Logger: Warn about radius_bottom
                    end
                    alt r_top < min_radius
                        Validator->>Logger: Warn about radius_top
                    end
                else not tapered
                    alt r_bottom < min_radius
                        Validator->>Logger: Warn about radius
                    end
                end
            end
        end
    end
    
    Validator-->>HeatChargeSimulation: Return validated structures
    HeatChargeSimulation-->>User: Simulation created (with warnings)
Loading

Context used:

  • Rule from dashboard - Use a tolerance-based comparison (e.g., np.isclose) for floating-point numbers instead of direct equ... (source)
  • Rule from dashboard - Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)

Copilot AI review requested due to automatic review settings January 29, 2026 11:29
@marc-flex marc-flex self-assigned this Jan 29, 2026
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.

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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 pull request adds validation to warn users when cylinder radii are too small for gmsh meshing in heat-charge simulations. The feature helps prevent meshing issues by alerting users to geometries that may cause problems during the meshing process.

Changes:

  • Added MIN_GMSH_RADIUS constant (1e-6) representing OpenCASCADE tolerance
  • Implemented _warn_small_cylinder_radius validator that checks cylinder radii and warns when they fall below meshing thresholds
  • Added comprehensive tests covering both non-tapered and tapered cylinder scenarios

Reviewed changes

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

File Description
tidy3d/components/tcad/simulation/heat_charge.py Added imports for Cylinder and traverse_geometries, defined MIN_GMSH_RADIUS constant, and implemented validator to warn about small cylinder radii
tests/test_components/test_heat_charge.py Added test function to verify warnings are issued for small cylinder radii in both non-tapered and tapered cases

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

@marc-flex marc-flex force-pushed the feat/FXC-5154-gmsh-min-cylinder-radii branch from 12b4bef to c91f81b Compare January 29, 2026 11:39
@marc-flex marc-flex force-pushed the feat/FXC-5154-gmsh-min-cylinder-radii branch from c91f81b to 61e8eb7 Compare January 29, 2026 11:54
@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2026

Diff Coverage

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

  • tidy3d/components/geometry/utils.py (100%)
  • tidy3d/components/tcad/simulation/heat_charge.py (95.2%): Missing lines 392

Summary

  • Total: 23 lines
  • Missing: 1 line
  • Coverage: 95%

tidy3d/components/tcad/simulation/heat_charge.py

Lines 388-396

  388 
  389                     # Warn if radii are below minimum
  390                     if is_tapered:
  391                         if r_bottom < min_radius:
! 392                             log.warning(
  393                                 f"Cylinder 'radius_bottom' ({r_bottom:.3e}) is below the minimum "
  394                                 f"radius for meshing ({min_radius:.3e}). The sidewall angle may be "
  395                                 f"too steep. Will be clamped to minimum radius or mesh size, whichever is larger."
  396                             )

@marc-flex marc-flex force-pushed the feat/FXC-5154-gmsh-min-cylinder-radii branch from 61e8eb7 to 9b429c9 Compare January 29, 2026 13:02
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.

@marc-flex marc-flex force-pushed the feat/FXC-5154-gmsh-min-cylinder-radii branch from 9b429c9 to 1c79e74 Compare January 29, 2026 13:50
@marc-flex marc-flex force-pushed the feat/FXC-5154-gmsh-min-cylinder-radii branch from 1c79e74 to 0a0063e Compare February 4, 2026 12:05
@marc-flex marc-flex requested review from momchil-flex and removed request for momchil-flex February 4, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants