allow mmi imbalance #386
allow mmi imbalance #386
Conversation
joamatab
commented
Apr 23, 2024
- add mmi_nxn
- add model for mmi imbalance
There was a problem hiding this comment.
Hey @joamatab - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| loss_dB_cross: Float | None = None, | ||
| loss_dB_thru: Float | None = None, | ||
| splitting_ratio_cross: Float = 0.5, | ||
| splitting_ratio_thru: Float = 0.5, |
There was a problem hiding this comment.
issue (testing): Tests needed for new parameters in mmi2x2 function.
The mmi2x2 function now includes additional parameters such as loss_dB_cross, loss_dB_thru, splitting_ratio_cross, and splitting_ratio_thru. It's important to verify that these parameters are handled correctly and interact as expected with the existing parameters. Please add tests to validate these interactions and their impact on the output.
| def _mmi_nxn( | ||
| n, | ||
| wl=1.55, | ||
| wl0=1.55, | ||
| fwhm=0.2, | ||
| loss_dB=None, | ||
| shift=None, | ||
| splitting_matrix=None, | ||
| ) -> sax.SDict: | ||
| """ | ||
| General n x n MMI model. | ||
|
|
||
| Args: | ||
| n (int): Number of input and output ports. | ||
| wl (float): Operating wavelength. | ||
| wl0 (float): Center wavelength of the MMI. | ||
| fwhm (float): Full width at half maximum. | ||
| loss_dB (np.array): Array of loss values in dB for each port. | ||
| shift (np.array): Array of wavelength shifts for each port. | ||
| splitting_matrix (np.array): nxn matrix defining the power splitting ratios between ports. | ||
| """ | ||
| if loss_dB is None: | ||
| loss_dB = jnp.zeros(n) | ||
| if shift is None: | ||
| shift = jnp.zeros(n) | ||
| if splitting_matrix is None: | ||
| splitting_matrix = jnp.full((n, n), 1 / n) # Uniform splitting as default | ||
|
|
||
| S = {} | ||
| for i in range(n): | ||
| for j in range(n): | ||
| amplitude = _mmi_amp(wl, wl0 + shift[j], fwhm, loss_dB[j]) | ||
| amplitude *= jnp.sqrt( | ||
| splitting_matrix[i][j] | ||
| ) # Convert power ratio to amplitude | ||
| loss_factor = 10 ** (-loss_dB[j] / 20) | ||
| S[(f"o{i+1}", f"o{j+1}")] = amplitude * loss_factor | ||
|
|
||
| return sax.reciprocal(S) |
There was a problem hiding this comment.
issue (testing): Missing tests for the new _mmi_nxn function.
The addition of the _mmi_nxn function introduces significant new functionality. It's crucial to ensure this function behaves as expected under various conditions, including edge cases such as extreme values for n, wl, wl0, fwhm, loss_dB, shift, and splitting_matrix. Please add unit tests to cover these scenarios.