Skip to content

Add taper support and refactor array-factor calculation #2726

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

George-Guryev-flxcmp
Copy link
Contributor

@George-Guryev-flxcmp George-Guryev-flxcmp commented Aug 8, 2025

Add taper support to RectangularAntennaArrayCalculator; refactor array factor computation for clarity and efficiency.

  • Implemented taper integration
  • Refactored AF calculation
  • Updated tests and docs

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.

4 files reviewed, 7 comments

Edit Code Review Bot Settings | Greptile

Copy link
Contributor

github-actions bot commented Aug 8, 2025

Diff Coverage

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

  • tidy3d/plugins/microwave/array_factor.py (92.8%): Missing lines 654,703,917,937,957,977,1030,1196,1320,1332

Summary

  • Total: 139 lines
  • Missing: 10 lines
  • Coverage: 92%

tidy3d/plugins/microwave/array_factor.py

Lines 650-658

  650             amp_z = amp_z[:, None, None]
  651 
  652         # Tapers with non-separable amplitude weights are not supported by this function
  653         else:
! 654             raise ValueError(f"Unsupported taper type {type(self.taper)} was passed.")
  655 
  656         # Calculate individual array factors in x, y, and z directions
  657         af_x = np.sum(
  658             amp_x * exp_x,

Lines 699-707

  699         amps = self.taper.amp_multipliers(self.array_size)
  700 
  701         # ensure amplitude weights are in format tuple[ArrayLike, ]
  702         if len(amps) != 1:
! 703             raise ValueError(
  704                 "Non-cartesian taper was expected. Please ensure a valid taper is used."
  705             )
  706 
  707         # compute array factor: AF(theta,f) = sum_{x,y,z} amp(x,y,z) * exp_x(x,theta,f)*exp_y(y,theta,f)*exp_z(z,theta,f)

Lines 913-921

  913         -------
  914         ArrayLike
  915             1D array of Hamming window weights.
  916         """
! 917         return hamming(N)
  918 
  919 
  920 class BlackmanWindow(AbstractWindow):
  921     """Standard Blackman window for tapering or spectral shaping."""

Lines 933-941

  933         -------
  934         ArrayLike
  935             1D array of Blackman window weights.
  936         """
! 937         return blackman(N)
  938 
  939 
  940 class BlackmanHarrisWindow(AbstractWindow):
  941     """Standard Blackman-Harris window for tapering or spectral shaping."""

Lines 953-961

  953         -------
  954         ArrayLike
  955             1D array of Blackman-Harris window weights.
  956         """
! 957         return blackmanharris(N)
  958 
  959 
  960 class HannWindow(AbstractWindow):
  961     """Hann window with configurable sidelobe suppression and sidelobe count."""

Lines 973-981

  973         -------
  974         ArrayLike
  975             1D array of Hann window weights.
  976         """
! 977         return hann(N)
  978 
  979 
  980 class ChebWindow(AbstractWindow):
  981     """Standard Chebyshev window for tapering with configurable sidelobe attenuation."""

Lines 1026-1034

  1026         -------
  1027         ArrayLike
  1028             1D array of Kaiser window weights.
  1029         """
! 1030         return kaiser(N, self.beta)
  1031 
  1032 
  1033 class _Taylor1D(AbstractWindow):
  1034     """Taylor window with configurable sidelobe suppression and sidelobe count."""

Lines 1192-1200

  1192             Weights sampled from the Taylor window.
  1193         """
  1194         if self.mode == "1d":
  1195             if not isinstance(arg, int):
! 1196                 raise TypeError("1D Taylor expects integer size.")
  1197             return _Taylor1D(sll=self.sll, nbar=self.nbar).get_weights(N=arg)
  1198         else:
  1199             locs = np.asarray(arg)
  1200             return _TaylorRadial(sll=self.sll, nbar=self.nbar).get_weights(p_vec=locs)

Lines 1316-1324

  1316     # Change TaylorWindow mode to call _TaylorRadial
  1317     @pd.root_validator(pre=False)
  1318     def force_radial_taylor(cls, values):
  1319         if not values:
! 1320             return values
  1321 
  1322         w = values.get("window")
  1323 
  1324         # Allow TaylorWindow instance; reject anything else early

Lines 1328-1336

  1328             )
  1329 
  1330         # Ensure that "radial" mode is used in ``RadialTaper``
  1331         if w.mode != "radial":
! 1332             raise TypeError(
  1333                 f'Expected a TaylorWindow with `mode\' set to "radial", got "{w.mode}".'
  1334             )
  1335 
  1336         return values

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

Great start! Still not sure about at, sll, nbar, etc vs attenuation, side_lobe_level, num_constant_sidelobes, etc. @yaugenst what's our policy on using pydantic alias feature on frontend?

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Very nice work!


# validate that at least one window is provided
@classmethod
def all_dims(cls, window: RectangularWindowType) -> RectangularTaper:
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe isotropic_window for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would make it clearer. Changed to isotropic_window()

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, maybe from_isotropic_window make more sense

… calculation

- Implemented taper integration to allow amplitude weighting in antenna arrays
- Refactored array factor computation for improved clarity and efficiency
- Updated relevant tests and documentation accordingly
Comment on lines +1160 to +1164
mode: Literal["1d", "radial"] = pd.Field(
default="1d",
title="Mode of Taylor Window",
description="Desired mode of Taylor window.",
)
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute Aug 14, 2025

Choose a reason for hiding this comment

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

Hmm, is there a way we can avoid this mode switch? What if we just define

class AbstractWindow(Tidy3dBaseModel, ABC):
    """This class provides interface for window selection."""

    def _get_weights_discrete(self, N: int) -> ArrayLike:
        """Interface function for computing window weights at N points."""
        raise Tidy3DNotImplemented(f"Calculation of antenna amplitudes at a discrete number of points is not yet implemented for window type {self.type}.")

    def _get_weights_continuous(self, p_vec: ArrayLike) -> ArrayLike:
        """Interface function for computing window weights at given locations."""
        raise Tidy3DNotImplemented(f"Calculation of antenna amplitudes at arbitrary locations is not yet implemented for window type {self.type}.")

then RectangularTaper can call _get_weights_discrete() while RadialTaper can call _get_weights_continuous(). TaylorWindow would have valid implementation for both, while all the others would only have _get_weights_discrete() for now. Would that work here? I think that would simplify the code


# Change TaylorWindow mode to call _TaylorRadial
@pd.root_validator(pre=False)
def force_radial_taylor(cls, values):
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really need a root validator here?

if not isinstance(w, TaylorWindow):
raise TypeError(
f"Expected a TaylorWindow instance for 'window', got {type(w).__name__}."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we don't need to validate this, as it should be validated already in the field's type declaration

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