Added ring modulator and test #28
Conversation
Reviewer's Guide by SourceryThis pull request introduces a Sequence diagram for RingModulator.process_waveformsequenceDiagram
participant User
participant RingModulator
participant RC Filter
User->>RingModulator: process_waveform(input_waveform, dt, wavelength_offset, voltage_waveform)
RingModulator->>RingModulator: reset()
RingModulator->>RingModulator: setup_sampling(dt)
alt voltage_waveform is not None and rc_filter_enabled
RingModulator->>RC Filter: Apply RC filter to voltage_waveform
RC Filter-->>RingModulator: filtered_voltage
else voltage_waveform is None or rc_filter_enabled is false
RingModulator->>RingModulator: filtered_voltage = voltage_waveform
end
loop for each sample in input_waveform
RingModulator->>RingModulator: Calculate phase (phi)
RingModulator->>RingModulator: Get delayed output from buffer
RingModulator->>RingModulator: Calculate new transmission (T_current)
RingModulator->>RingModulator: Store T_current in buffer
RingModulator->>RingModulator: Calculate output
end
RingModulator-->>User: output_waveform
Class diagram for RingModulatorclassDiagram
class RingModulator{
-radius: float
-resonant_wavelength: float
-n_eff: float
-ng: float
-dn_dV: float
-a: float
-kappa: float
-buffer_size: int
-rc_filter_enabled: bool
-rc_time_constant: float
-sigma: float
-Lrt: float
-junction_loss_dB_m: float
-tau: float
-phase_offset: float
-buffer: numpy.ndarray
-buffer_idx: int
-buffer_initialized: bool
-previous_filtered_voltage: float
+__init__(
radius=10e-6,
resonant_wavelength=1550e-9,
n_eff=2.4,
ng=4.2,
dn_dV=2E-4,
a=4000,
kappa=0.1,
buffer_size=1000000,
rc_filter_enabled=False,
rc_time_constant=1e-9
)
+reset()
+FSR
+finesse
+FWHM
+quality_factor
+photon_lifetime
+setup_sampling(dt)
+process_waveform(input_waveform, dt, wavelength_offset=0.0, voltage_waveform=None)
+calculate_phase(wavelength, voltage=0)
+plot_frequency_response(lambda_start=1500e-9, lambda_end=1600e-9, points=1000, voltage=0)
}
note for RingModulator "Represents an optical ring resonator with bus coupling.
Calculates both the time domain (transient) and frequency domain (steady state) response."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @alexsludds - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a unit test or example demonstrating the usage of the
RingModulatorclass. - It might be helpful to include a diagram or visual representation of the ring modulator to aid understanding.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
optic/models/devices.py
Outdated
| if buffer_samples > self.buffer_size: | ||
| self.buffer = np.zeros(buffer_samples * 2, dtype=complex) | ||
| self.buffer_size = buffer_samples * 2 | ||
| print(f"Warning: Buffer size increased to {self.buffer_size} to accommodate delay") |
There was a problem hiding this comment.
suggestion: Consider using a logging framework instead of print statements.
For production-level code, using a logging mechanism would allow better control over the verbosity and destination of such messages. This is especially useful for warning messages like buffer size adjustments.
Suggested implementation:
import numpy as np
import logging logging.warning(f"Buffer size increased to {self.buffer_size} to accommodate delay")If your codebase already configures logging elsewhere, ensure that the configuration (log level, handlers, etc.) is compatible with this usage.
optic/models/devices.py
Outdated
| resonant_wavelength=1550e-9, | ||
| n_eff=2.4, | ||
| ng = 4.2, | ||
| dn_dV = 2E-4, |
There was a problem hiding this comment.
nitpick (typo): Documentation mismatch in parameter naming.
The init docstring refers to the effective index parameter with the description 'rseonant wavelength' and also uses 'n_g' in the description instead of 'ng'. Aligning the parameter name and correcting the typo will improve the documentation clarity.
Suggested implementation:
resonant wavelength : float
The effective operating wavelength of the resonator in meters. ng : float
The group index of the resonator.Review the entire init docstring to ensure that all other parameter names match their variable names in the signature.
optic/models/devices.py
Outdated
| # Pre-compute voltage scaling factor | ||
| voltage_factor = 2*np.pi/self.resonant_wavelength * self.dn_dV * self.Lrt | ||
|
|
||
| # Pre-process voltage waveform if RC filter is enabled |
There was a problem hiding this comment.
issue (complexity): Consider using scipy.signal.lfilter for RC filtering and extracting the transmission computation into a helper function to improve code clarity and reduce manual bookkeeping within the waveform processing loop.
Consider simplifying the RC filtering and decoupling state updates from the phase/voltage logic. For example, rather than an explicit loop, you can use signal processing routines like scipy.signal.lfilter for the RC filter:
from scipy.signal import lfilter
# Replace RC filter loop with vectorized filtering:
if voltage_waveform is not None and self.rc_filter_enabled:
alpha = dt / (self.rc_time_constant + dt)
# Filter transfer function coefficients for first-order IIR filter
filtered_voltage = lfilter([alpha], [1, -(1 - alpha)], voltage_waveform)
else:
filtered_voltage = voltage_waveformAdditionally, consider isolating the transmission computation within the waveform processing loop to reduce coupling. For instance, extract the per-sample calculation into a helper function:
def compute_transmission(self, T_delayed, phi):
"""Compute the new transmission value."""
return self.sigma + self.a * np.exp(-1j * phi) * (self.sigma * T_delayed - 1)Then update the loop as follows:
for i in range(n_samples):
# Calculate phase for this sample
phi = base_phi
if voltage_waveform is not None:
phi += voltage_factor * filtered_voltage[i]
T_delayed = T_buffer[i]
T_current = self.compute_transmission(T_delayed, phi)
T_buffer[i + buffer_samples] = T_current
output_waveform[i] = input_waveform[i] * T_currentThese targeted changes will preserve functionality while reducing manual bookkeeping and improving overall clarity.
|
Hey @alexsludds, that's a great idea and example! |
|
Hello, @alexsludds! That's great you have a contribution to the repository! :) Please, note that OptiCommPy is a function-based instead of a class-based framework. The choice for functions instead of classes is to make the use of the code as simple as possible. So, please, I would like to ask you to make the following changes in your code:
Let me know if you have any questions! |
|
@edsonportosilva I updated the ring to be a function that takes in parameters. Please take a look! |
|
Hi, @alexsludds. Great job! It seems the model is working fine, at least it ran without problems here. Two things to before we can merge the pull request:
|
|
@alexsludds, another small fix: in the notebook you have created a new class to define the parameters, but instead you just need to instantiate a |
This pull request adds a ring modulator device that includes accurate models of the time dynamics/optical peaking of the ring.
Values are set to reasonable values based on literature/my experience for rings targeting ~50Gbaud.
Future work would include looking at ways to speedup the ring and adding examples of PAM-4 for a ring modulator.
Summary by Sourcery
Adds a ring modulator device with accurate models of the time dynamics and optical peaking of the ring. Includes parameters for rings targeting ~50Gbaud.
New Features: