Skip to content

Conversation

@marcorudolphflex
Copy link
Contributor

@marcorudolphflex marcorudolphflex commented Oct 15, 2025

This PR refactors the color mapping logic in plot_eps to align with standard Matplotlib semantics and removes the temporary fix which inverts at two places in code.

What changed
Set the (inverted) cmap color directly as facecolor and choose right cmap in _pcolormesh_shape_custom_medium_structure_eps

Greptile Overview

Updated On: 2025-10-15 12:29:14 UTC

Greptile Summary

This PR refactors the color mapping logic in the plot_eps functionality to fix color computation inconsistencies and align with standard Matplotlib semantics. The changes introduce a centralized _get_colormap() helper function to handle colormap selection based on the reverse parameter, eliminating code duplication across multiple plotting functions. The refactor removes temporary workarounds that manually inverted colors through mathematical operations (eps_min + eps_max - eps) and replaces them with proper matplotlib colormap usage. The most significant change is in the structure epsilon plotting parameters, where complex color computation logic is replaced with standard matplotlib normalization and colormap application, directly setting RGBA tuples as facecolor values. These changes ensure consistent color mapping behavior across different epsilon visualization functions while leveraging matplotlib's built-in colormap functionality.

Changed Files
Filename Score Overview
tidy3d/components/scene.py 4/5 Refactors color mapping logic by adding _get_colormap() helper function, removing manual color inversion hacks, and implementing proper matplotlib colormap usage for epsilon visualization
tests/test_components/test_scene.py 5/5 Adds comprehensive test coverage for color mapping functionality including proper matplotlib colormap comparison and validation of both normal and reversed colormap scenarios

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it primarily refactors existing functionality to use standard matplotlib patterns
  • Score reflects solid implementation of matplotlib best practices but potential edge cases around colormap edge values and backward compatibility concerns with existing plot customizations
  • Pay close attention to tidy3d/components/scene.py for the color value clamping logic and ensure all epsilon visualization functions handle edge cases properly

Sequence Diagram

sequenceDiagram
    participant User
    participant Scene
    participant Matplotlib as mpl
    participant ColorMap as cmap
    participant PlotParams

    User->>Scene: plot_eps(reverse=False/True)
    Scene->>Scene: _get_structure_eps_plot_params()
    Scene->>Scene: _get_colormap(reverse)
    
    alt reverse is False
        Scene->>cmap: STRUCTURE_EPS_CMAP
    else reverse is True
        Scene->>cmap: STRUCTURE_EPS_CMAP_R
    end
    
    Scene->>mpl: colors.Normalize(vmin, vmax)
    Scene->>Scene: medium._eps_plot(freq)
    Scene->>mpl: norm(eps_medium)
    Scene->>cmap: get_cmap(cmap_name)
    Scene->>cmap: cmap(color_value)
    Scene->>PlotParams: copy(facecolor=rgba)
    PlotParams-->>Scene: updated plot_params
    Scene->>Scene: plot_shape(shape, plot_params)
    Scene-->>User: rendered plot
Loading

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, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/components/scene.py (90.9%): Missing lines 1323

Summary

  • Total: 11 lines
  • Missing: 1 line
  • Coverage: 90%

tidy3d/components/scene.py

Lines 1319-1327

  1319                         eps = eps.plane_slice(axis=normal_axis_ind, pos=normal_position)
  1320 
  1321                     # at this point eps_mean is TriangularGridDataset and we just plot it directly
  1322                     # with applying shape mask
! 1323                     cmap_name = _get_colormap(reverse=reverse)
  1324                     eps.plot(
  1325                         grid=False,
  1326                         ax=ax,
  1327                         cbar=False,

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

@damiano-flex
Copy link
Contributor

damiano-flex commented Oct 15, 2025

LGTM

I also checked this works in the notebook where @momchil-flex initially found the issue.

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 15, 2025
Merged via the queue into develop with commit 707496a Oct 15, 2025
58 of 75 checks passed
@yaugenst-flex yaugenst-flex deleted the FXC-3655-fix-the-color-computation-for-plot-eps branch October 15, 2025 14:48
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