Add FreqRange utility class and unit tests (related issue #2529) #2542

George-Guryev · 2025-06-05T04:39:37Z

Summary

This pull request implements a new utility class FreqRange for convenient handling of frequency and wavelength conversions in simulations.

Related issue

Convenience class for handling frequency/wavelength conversion and assignment #2529

What was implemented

Created FreqRange class to:
- Accept central frequency and bandwidth as input (SI units).
- Provide computed fmin, fmax, freqs, and ldas.
- Support construction from wavelength ranges and intervals.
- Include sampling method for generating frequency arrays.
- Provide compatibility with GaussianPulse.
Added unit tests in tests/test_FreqRange.py to validate:
- Constructor and class methods (frequency- and wavelength-based initializations)
- Sampling behavior
- Instantiation of GaussianPulse with given central frequency and bandwidth

Includes methods for frequency/wavelength conversion, interval updates, and sampling. Adds test coverage for main methods.

momchil-flex

Hey, this is looking really good, and the tests are thorough!

I do have some comments which are mainly related to the way things work in Tidy3D specifically. You have used a more generic way to define things in python, while our components use pydantic to define arguments, and inherit from Tidy3dBaseModel.

Regarding using SI units: since everything in Tidy3D uses Tidy3D units, I think so should the FreqRange and all its methods.

momchil-flex · 2025-06-05T12:33:37Z

tidy3d/freq_range.py

+        """
+
+        # create an instance of GaussianPulse class with defined frequency params
+        return GaussianPulse(freq0 = self.freq0, fwidth = self.fwidth)


Better to use from_frequency_range here because this method ensures a good amount of power in the pulse signal across the frequency range.

Thank you for pointing this out! Fixed it

momchil-flex · 2025-06-05T12:42:38Z

tidy3d/freq_range.py

+        if not isinstance(freq0, (int, float)):
+            raise TypeError("Central frequency freq0 must be a number (int or float)")
+        if not isinstance(fwidth, (int, float)):
+            raise TypeError("Frequency bandwidth fwidth must be a number (int or float)")


Generally, Tidy3D follows a specific development pattern for all components. Everything is a subclass of Tidy3dBaseModel, which is immutable and does not store an internal state. There is also no init method. The input fields are defined as pydantic fields (which takes care of the type validation), and there are no private attributes. See e.g. here, but any component, really.

So here, you should decide what would be the fundamental input arguments: for example, freq0 and fwidth, and everything else should be @property rather than assigned in the __init__ method. Similarly there would either be no self.num_points, or if there is one, then it should be static and should be removed as an argument in the samples method.

I think one thing that is missing in the implementation is to be able to have the points either equally spaced in frequency, or equally spaced in wavelength. This could either be an argument passed to the FreqRange, or, maybe better, just an argument to samples.

Rewrote class using pydantic. Looks cleaner now. Thank you for walking me through the development pattern, it was helpful!

tylerflex

Thanks @George-Guryev , looks good overall! A few things we can work on to make the design more in line with our existing design

tylerflex · 2025-06-05T13:30:58Z

tests/test_FreqRange.py

We generally separate these tests into different sub directories. for this test, I suggest putting inside of tests/test_components/

Makes sense, fixed it!

tylerflex · 2025-06-05T13:31:43Z

tests/test_FreqRange.py

@@ -0,0 +1,134 @@
+import numpy as np 


we also generally add a short module level docstring to describe what this file is testing, see other test files for examples.

tylerflex · 2025-06-05T13:34:15Z

CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [Unreleased]

+### Added
+- Implemented `FreqRange` utility class for frequency/wavelength handling; the default constructor and `from_interval()` class method assume, that the central frequency `freq0` is defined to be `freq0 = (fmin + fmax) / 2`; class methods `from_wavelength()` and `from_wavelegth_interval()` assume that the central wavelength `lda0` is defined to be `lda0 = (lda_min + lda_max) / 2`. 


I think all three of these bullets could be combined into one, potentially. something like

- Utility class `FreqRange` for frequency and wavelength handling. With constructor methods x, y, and z to initialize frequency data.

I think the details about the arguments can be left out of the changelog for now since they are part of the API reference already. And we wont need to mention that we added unit tests in the changelog.

tylerflex · 2025-06-05T13:35:29Z

tidy3d/freq_range.py

Please add a module level docstring at the top of the file to briefly describe what is in this module.

tylerflex · 2025-06-05T13:36:33Z

tidy3d/freq_range.py

+    return val * 1e-6
+
+
+class FreqRange:


as Momchil mentioned, we use pydantic to define classes that store data, which also handles initialization. see this for an example

tidy3d/tidy3d/components/apodization.py

Line 16 in d354c66

class ApodizationSpec(Tidy3dBaseModel):

tylerflex · 2025-06-05T13:39:41Z

tidy3d/freq_range.py

+    #       class methods 
+    #-----------------------------------
+    @classmethod
+    def from_interval(clf, fmin, fmax):


please be sure to add type hints to these and other function signatures, eg fmin: float

Also, in python we usually call the class cls.

Thank you for pointing these issues out. Fixed both

tylerflex · 2025-06-05T13:46:30Z

tidy3d/freq_range.py

+
+        freq_range = clf(freq0, fwidth)
+
+        freq_range.fmin = fmin


these are already set in the init method? or is there a need to overwrite their values here?

There was an issue with an ambiguous definition of what "central wavelength" and "central frequency" were:

Naturally one interprets central frequency f0 = (fmin + fmax)/ 2 and central wavelength lda0 = (lda_min + lda_max)/2, i.e. both quantities are points in the middle of respective frequency and wavelength ranges.
If we assume that the dispersion equation holds: |k| = omega / c, then wavelength would be inverse-proportional to the corresponding frequency: lambda = c / f. In this case central wavelength does NOT map to the central frequency (and vice versa).

However, the first two lines in the ticket #2529 suggest the following definition:

lda0 = 0.8 # central wavelength
freq0 = td.C_0 / lda0 # central frequency

therefore the "central frequency" freq0 that corresponds to lda0 is not a true central frequency (i.e. it does not lie in the center of the frequency range [fmin, fmax].

Assumption in first commit

In first commit I decided to stick with the ambiguity in definition of "central frequency" and "central wavelength": if user constructs object with frequency args, freq0 is the true central frequency, while lda0 is defined as lda0 = c / freq0, but this "central wavelength" is not a true central wavelength; if user specifies wavelengths - reverse the argument.

To reflect that I had to modify fmin after constructor, as one could not sample frequencies with fwidth without reference frequency.

Assumption in second commit

I was not entirely happy with the solution and decided to change the assumption:
Instead I used the intuitive definitions for central wavelength and central frequency and did not enforce lda0 = c / freq0. This removes unnecessary ambiguity in definition and the need to reassign properties after object construction

tylerflex · 2025-06-05T13:51:49Z

tidy3d/freq_range.py

+                raise ValueError("frequencies have to be positive values")
+
+            # calculate frequency points and corresponding wavelengths
+            self.freqs = np.linspace(self.fmin, self.fmax, self.num_points)


is there a need to store self.freqs and self.ldas? if samples is meant to simply return a numpy array of frequencies, perhaps we shouldn't be modifying self at all?

it might be worth considering a design where instead of samples(num_points) we simply expose two methods FreqRange.freqs(num_points) and FreqRange.wavelengths(num_points)?

That's a great suggestion! I've replaced samples(num_points) with freqs(num_points) and ldas(num_points). Looks cleaner now.

tylerflex · 2025-06-05T13:52:17Z

tidy3d/freq_range.py

+
+        return self.freqs
+
+    def gaussian_pulse(self):


what about if I want to pass other arguments that get passed to GaussianPulse? such as the amplitude?

tylerflex

please also be sure to add the FreqRange class to tidy3d/__init__.py, otherwise it wont be able to be imported from the tidy3d namespace (eg import tidy3d as td; td.FreqRange)

tylerflex

Thanks George!

I'm going to leave my 2nd round of comments here just so they are recorded and I dont lose them. But for the purposes of this exercise, feel free to leave the PR as is (unless you feel compelled to make the changes), we can always pick this up later.

tylerflex · 2025-06-09T14:24:12Z

tidy3d/components/source/freq_range.py

+        freq0  = 0.5 * (fmax + fmin)
+        fwidth = 0.5 * (fmax - fmin)
+
+        return cls(freq0=freq0, fwidth=fwidth)


I would prefer maybe to just call cls.from_interval using fmin and fmax, to avoid duplicating operations

tylerflex · 2025-06-09T14:25:22Z

tidy3d/components/source/freq_range.py

+    #       class methods 
+    #-----------------------------------
+    @classmethod
+    def from_interval(cls, fmin: float, fmax: float) -> FreqRange:


we have from_interval and from_wavelength but both construct the FreqRange out of intervals. Perhaps to unify the naming, we should call these from_freq_interval and from_wvl_interval or similar?

tylerflex · 2025-06-09T14:25:58Z

tidy3d/components/source/freq_range.py

+        return cls(freq0=freq0, fwidth=fwidth)
+
+
+    def freqs(self, num_points: int)->NDArray[np.float64]:


nice, I think this api looks a lot cleaner 👍

tylerflex · 2025-06-09T14:26:57Z

tidy3d/components/source/freq_range.py

+            # generate array of wavelengths (in ascending order) 
+            return np.linspace(lmin, lmax, num_points)
+
+    def gaussian_pulse(self, **kwargs)->GaussianPulse:


let's call this to_gaussian_pulse() (just because internally we tend to use to_ and from_ for these kinds of conversion methods).

tylerflex · 2025-06-09T14:28:14Z

tidy3d/components/source/freq_range.py

+            with frequency-specific parameters defined in 'FreqRange'
+        """
+        # create an instance of GaussianPulse class with defined frequency params
+        return GaussianPulse.from_frequency_range(self.fmin, self.fmax, **kwargs)


let's pass things using the keywords, eg (from_frequency_range(fmin=self.fmin, ...) for extra robustness.

tylerflex · 2025-06-09T14:28:57Z

CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [Unreleased]

+### Added
+- Implemented `FreqRange` utility class for frequency/wavelength handling with constructor methods `from_interval()`, `from_wavelength()`, and `from_wavelength_interval()`. 
+- Added methods `freqs(num_points)` and `ldas(num_points)` return numpy arrays of frequency and wavelength points uniformly sampled from the specified frequency range. 


I think we can just remove these second two bullets, the first should be sufficient. and users can check the API reference in the docs for more details.

tylerflex · 2025-06-09T14:29:11Z

tidy3d/components/source/freq_range.py

+        1.0, title="Central frequency", description="Real-valued positive central frequency", units="Hz")
+
+    fwidth: pydantic.PositiveFloat = pydantic.Field(
+        1.0, title="Frequency bandwidth", description="Real-valued positive central frequency", units="Hz") 


description needs updating too.

tylerflex · 2025-06-09T14:29:24Z

tidy3d/components/source/freq_range.py

+
+            Input Parameters
+            ----------------
+            wvl0      - central wavelength  


we'll need to update the docstrings to be consistent with our formatting.

Add FreqRange utility class and unit tests

d354c66

Includes methods for frequency/wavelength conversion, interval updates, and sampling. Adds test coverage for main methods.

momchil-flex requested review from tylerflex and momchil-flex June 5, 2025 07:50

momchil-flex reviewed Jun 5, 2025

View reviewed changes

tylerflex requested changes Jun 5, 2025

View reviewed changes

tylerflex reviewed Jun 5, 2025

View reviewed changes

modify implementation of class FreqRange to use pydantic

6d7771a

tylerflex requested changes Jun 9, 2025

View reviewed changes

		return cls(freq0=freq0, fwidth=fwidth)


		def freqs(self, num_points: int)->NDArray[np.float64]:

Add FreqRange utility class and unit tests (related issue #2529) #2542

Are you sure you want to change the base?

Add FreqRange utility class and unit tests (related issue #2529) #2542

Uh oh!

Conversation

George-Guryev commented Jun 5, 2025

Summary

Related issue

What was implemented

Uh oh!

momchil-flex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerflex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Assumption in first commit

Assumption in second commit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerflex left a comment

Choose a reason for hiding this comment

Uh oh!

tylerflex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!