Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Jan 6, 2026

Warnings were being emitted by xarray/numpy when plotting with scale=dB.


Note

Prevents divide-by-zero warnings when plotting fields with scale='dB' by safely handling zeros.

  • Introduces AbstractSimulationData._apply_log_scale() to replace zeros (respecting optional vmin) and apply log10 with appropriate db_factor
  • Refactors plot_field_monitor_data to use the new helper for dB scaling
  • Adds tests covering zero-valued data with and without vmin to ensure no warnings
  • Updates CHANGELOG.md under Fixed

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

Greptile Overview

Greptile Overview

Greptile Summary

This PR fixes divide-by-zero warnings when plotting field data with scale=dB by replacing zero values before applying log10. The implementation correctly handles both the case where vmin is specified (zeros map to the floor value) and when it's not (zeros map to NaN). The mathematical logic is sound: when vmin is provided, fill_val = 10 ** (vmin / db_factor) ensures that zeros ultimately map to exactly vmin dB after the log transformation.

Key Changes:

  • Modified plot_field_monitor_data to use xarray.where() to replace zeros while preserving NaN values
  • Added comprehensive test coverage for both vmin=None and vmin=-50 cases
  • Added changelog entry under Fixed section

Observations:

  • The fix maintains backward compatibility by preserving existing behavior for non-zero values
  • NaN preservation logic is correct: np.isnan(field_data) works properly with xarray DataArrays
  • Test uses warnings.filterwarnings("error") to ensure no divide-by-zero warnings are raised

Confidence Score: 5/5

  • This PR is safe to merge with high confidence
  • The implementation is mathematically correct and well-tested. The fix properly handles the edge cases (zeros, NaN, positive/negative vmin values). The code follows project conventions including proper use of is not None checks. Test coverage validates the fix prevents warnings in both scenarios (with and without vmin).
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/data/sim_data.py 5/5 Fixed zero-value handling in dB scale plotting by replacing zeros with NaN or floor value before log10, preventing divide-by-zero warnings. Implementation is mathematically correct.
tests/test_data/test_sim_data.py 4/5 Added test to verify no divide-by-zero warnings when plotting with dB scale and zero values. Tests both with and without vmin. Could benefit from explicit NaN value testing.
CHANGELOG.md 5/5 Added changelog entry under Fixed section for handling zero values in plot_field with dB scale. Entry follows format conventions with proper backticks.

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationData
    participant plot_field_monitor_data
    participant _field_component_value
    participant numpy
    participant xarray
    
    User->>SimulationData: plot_field(scale="dB")
    SimulationData->>plot_field_monitor_data: call with field_data
    plot_field_monitor_data->>_field_component_value: get field values (val="abs")
    _field_component_value->>numpy: np.abs(field_component)
    numpy-->>_field_component_value: absolute values
    _field_component_value-->>plot_field_monitor_data: field_data (xarray)
    
    alt scale == "dB"
        plot_field_monitor_data->>plot_field_monitor_data: determine db_factor
        
        alt vmin is not None
            plot_field_monitor_data->>plot_field_monitor_data: fill_val = 10 ** (vmin / db_factor)
        else vmin is None
            plot_field_monitor_data->>plot_field_monitor_data: fill_val = np.nan
        end
        
        plot_field_monitor_data->>numpy: np.abs(field_data)
        numpy-->>plot_field_monitor_data: absolute values
        
        plot_field_monitor_data->>xarray: field_data.where((field_data > 0) | np.isnan(field_data), fill_val)
        Note over xarray: Replace zeros with fill_val,<br/>preserve NaN values
        xarray-->>plot_field_monitor_data: field_data with zeros replaced
        
        plot_field_monitor_data->>numpy: db_factor * np.log10(field_data)
        Note over numpy: No divide-by-zero warnings<br/>because zeros were replaced
        numpy-->>plot_field_monitor_data: dB values
    end
    
    plot_field_monitor_data-->>User: plot with dB scale
Loading

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/data/sim_data.py 5/5 Fixed zero-value handling in dB scale plotting by replacing zeros with NaN or floor value before log10, preventing divide-by-zero warnings. Implementation is mathematically correct.
tests/test_data/test_sim_data.py 4/5 Added test to verify no divide-by-zero warnings when plotting with dB scale and zero values. Tests both with and without vmin. Could benefit from explicit NaN value testing.
CHANGELOG.md 5/5 Added changelog entry under Fixed section for handling zero values in plot_field with dB scale. Entry follows format conventions with proper backticks.

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationData
    participant plot_field_monitor_data
    participant _field_component_value
    participant numpy
    participant xarray
    
    User->>SimulationData: plot_field(scale="dB")
    SimulationData->>plot_field_monitor_data: call with field_data
    plot_field_monitor_data->>_field_component_value: get field values (val="abs")
    _field_component_value->>numpy: np.abs(field_component)
    numpy-->>_field_component_value: absolute values
    _field_component_value-->>plot_field_monitor_data: field_data (xarray)
    
    alt scale == "dB"
        plot_field_monitor_data->>plot_field_monitor_data: determine db_factor
        
        alt vmin is not None
            plot_field_monitor_data->>plot_field_monitor_data: fill_val = 10 ** (vmin / db_factor)
        else vmin is None
            plot_field_monitor_data->>plot_field_monitor_data: fill_val = np.nan
        end
        
        plot_field_monitor_data->>numpy: np.abs(field_data)
        numpy-->>plot_field_monitor_data: absolute values
        
        plot_field_monitor_data->>xarray: field_data.where((field_data > 0) | np.isnan(field_data), fill_val)
        Note over xarray: Replace zeros with fill_val,<br/>preserve NaN values
        xarray-->>plot_field_monitor_data: field_data with zeros replaced
        
        plot_field_monitor_data->>numpy: db_factor * np.log10(field_data)
        Note over numpy: No divide-by-zero warnings<br/>because zeros were replaced
        numpy-->>plot_field_monitor_data: dB values
    end
    
    plot_field_monitor_data-->>User: plot with dB scale
Loading

@dmarek-flex dmarek-flex self-assigned this Jan 6, 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.

3 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2026

Diff Coverage

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

  • tidy3d/components/base_sim/data/sim_data.py (100%)
  • tidy3d/components/data/sim_data.py (100%)

Summary

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

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4702-Handle-zeros-when-plotting-with-scale-dB branch 2 times, most recently from 16865b5 to fcc3743 Compare January 7, 2026 14:37
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4702-Handle-zeros-when-plotting-with-scale-dB branch from fcc3743 to dd065ae Compare January 7, 2026 15:04
@damiano-flex
Copy link
Contributor

@greptile Check the latest version of this PR and provide a new confidence score.

@damiano-flex
Copy link
Contributor

@greptileai Check the latest version of this PR and provide a new confidence score.

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

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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@damiano-flex damiano-flex left a comment

Choose a reason for hiding this comment

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

The PR looks OK 👍 I will make another one to fix the log scale which has the same issue.

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4702-Handle-zeros-when-plotting-with-scale-dB branch from dd065ae to d7b64f5 Compare January 9, 2026 17:46
@dmarek-flex dmarek-flex requested a review from marc-flex as a code owner January 9, 2026 17:46
@dmarek-flex
Copy link
Contributor Author

I will make another one to fix the log scale which has the same issue.

I went ahead and added the fix to the TCAD plot_field as well

@dmarek-flex
Copy link
Contributor Author

Ah I see now, I'll revert and let you fix the unstructured data set issues!

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-4702-Handle-zeros-when-plotting-with-scale-dB branch from d7b64f5 to 5ca813e Compare January 9, 2026 18:25
@dmarek-flex dmarek-flex enabled auto-merge January 9, 2026 18:26
@dmarek-flex dmarek-flex added this pull request to the merge queue Jan 9, 2026
Merged via the queue into develop with commit 9f1893a Jan 9, 2026
33 of 37 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/FXC-4702-Handle-zeros-when-plotting-with-scale-dB branch January 9, 2026 19:26
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.

3 participants