Skip to content

feat: add mil and inch units to plot_length_units #2655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 23, 2025

Conversation

yaugenst-flex
Copy link
Collaborator

@yaugenst-flex yaugenst-flex commented Jul 14, 2025

Partially addresses #2644

@yuanshen-flexcompute FYI we already had "in" in constants, but it wasn't registered as a LengthUnit for plots, hence I didn't rename it.

@yaugenst-flex yaugenst-flex self-assigned this Jul 14, 2025
@yaugenst-flex yaugenst-flex added the 2.9 will go into version 2.9.* label Jul 14, 2025
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.

Reviewing changes made in this pull request

Copy link
Contributor

github-actions bot commented Jul 14, 2025

Diff Coverage

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

  • tidy3d/components/types.py (100%)
  • tidy3d/components/viz/axes_utils.py (38.3%): Missing lines 29-30,34,38-39,42-43,45,47,50-51,53-56,61,64-65,70-71,73,76-77,80-81,84,91,94-95

Summary

  • Total: 48 lines
  • Missing: 29 lines
  • Coverage: 39%

tidy3d/components/viz/axes_utils.py

  25             super().__init__()
  26             self.scale_factor = scale_factor
  27 
  28         def __call__(self):
! 29             vmin, vmax = self.axis.get_view_interval()
! 30             return self.tick_values(vmin, vmax)
  31 
  32         def view_limits(self, vmin, vmax):
  33             """Override to prevent matplotlib from adjusting our limits."""
! 34             return vmin, vmax
  35 
  36         def tick_values(self, vmin, vmax):
  37             # convert the view range to the target unit
! 38             vmin_unit = vmin * self.scale_factor
! 39             vmax_unit = vmax * self.scale_factor
  40 
  41             # tolerance for floating point comparisons in target unit
! 42             unit_range = vmax_unit - vmin_unit
! 43             unit_tol = unit_range * 1e-8
  44 
! 45             locator = ticker.MaxNLocator(nbins=11, prune=None, min_n_ticks=2)
  46 
! 47             ticks_unit = locator.tick_values(vmin_unit, vmax_unit)
  48 
  49             # ensure we have ticks that cover the full range
! 50             if len(ticks_unit) > 0:
! 51                 if ticks_unit[0] > vmin_unit + unit_tol or ticks_unit[-1] < vmax_unit - unit_tol:
  52                     # try with fewer bins to get better coverage
! 53                     for n in [10, 9, 8, 7, 6, 5]:
! 54                         locator = ticker.MaxNLocator(nbins=n, prune=None, min_n_ticks=2)
! 55                         ticks_unit = locator.tick_values(vmin_unit, vmax_unit)
! 56                         if (
  57                             len(ticks_unit) >= 3
  58                             and ticks_unit[0] <= vmin_unit + unit_tol
  59                             and ticks_unit[-1] >= vmax_unit - unit_tol
  60                         ):
! 61                             break
  62 
  63                 # if still no good coverage, manually ensure edge coverage
! 64                 if len(ticks_unit) > 0:
! 65                     if (
  66                         ticks_unit[0] > vmin_unit + unit_tol
  67                         or ticks_unit[-1] < vmax_unit - unit_tol
  68                     ):
  69                         # find a reasonable step size from existing ticks
! 70                         if len(ticks_unit) > 1:
! 71                             step = ticks_unit[1] - ticks_unit[0]
  72                         else:
! 73                             step = unit_range / 5
  74 
  75                         # extend the range to ensure coverage
! 76                         extended_min = vmin_unit - step
! 77                         extended_max = vmax_unit + step
  78 
  79                         # try one more time with extended range
! 80                         locator = ticker.MaxNLocator(nbins=8, prune=None, min_n_ticks=2)
! 81                         ticks_unit = locator.tick_values(extended_min, extended_max)
  82 
  83                         # filter to reasonable bounds around the original range
! 84                         ticks_unit = [
  85                             t
  86                             for t in ticks_unit
  87                             if t >= vmin_unit - step / 2 and t <= vmax_unit + step / 2
  88                         ]

  87                             if t >= vmin_unit - step / 2 and t <= vmax_unit + step / 2
  88                         ]
  89 
  90             # convert the nice ticks back to the original data unit (micrometers)
! 91             ticks_um = ticks_unit / self.scale_factor
  92 
  93             # filter to ensure ticks are within bounds (with small tolerance)
! 94             eps = (vmax - vmin) * 1e-8
! 95             return [tick for tick in ticks_um if vmin - eps <= tick <= vmax + eps]
  96 
  97     return UnitAwareLocator
  98 

@yuanshen-flexcompute
Copy link

Overall looks good. One minor quibble: when using metric units, e.g. 'mm', the plot functions show nice round numbers in the axis ticks:
image

However, when switching to 'mil', the tick locations don't change, resulting in ugly numbers:
image

Is it possible to change the plot functions to show nicer tick numbers in 'mil' or 'in'?

Thanks.

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/units branch 2 times, most recently from e4249be to 75d0e08 Compare July 17, 2025 20:20
@yaugenst-flex yaugenst-flex linked an issue Jul 17, 2025 that may be closed by this pull request
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/units branch 3 times, most recently from e93243b to 51663ae Compare July 23, 2025 10:46
@yaugenst-flex yaugenst-flex removed the request for review from momchil-flex July 23, 2025 10:46
@yaugenst-flex
Copy link
Collaborator Author

@yuanshen-flexcompute fixed!

plot_mil

Copy link
Collaborator

@daquinteroflex daquinteroflex left a comment

Choose a reason for hiding this comment

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

Looks good! Users of imperial units will be thrilled

@yaugenst-flex yaugenst-flex removed the 2.9 will go into version 2.9.* label Jul 23, 2025
@yaugenst-flex yaugenst-flex added this pull request to the merge queue Jul 23, 2025
Merged via the queue into develop with commit a9bd090 Jul 23, 2025
28 of 31 checks passed
@yaugenst-flex yaugenst-flex deleted the yaugenst-flex/units branch July 23, 2025 11:34
@yaugenst-flex yaugenst-flex restored the yaugenst-flex/units branch July 23, 2025 11:41
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