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
Rename fiber ports #2700
Rename fiber ports #2700
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @joamatab - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
edge_coupler_silicon = partial( | ||
taper, | ||
width2=0.2, | ||
length=100, | ||
with_two_ports=True, | ||
port_order_types=("optical", "edge_te"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code_clarification): Consider documenting the addition of 'port_order_types' parameter.
Adding new parameters can affect how the function is used or its outputs. It's important to update the documentation to reflect these changes and provide guidance on how to use the new parameter.
edge_coupler_silicon = partial( | ||
taper, | ||
width2=0.2, | ||
length=100, | ||
with_two_ports=True, | ||
port_order_types=("optical", "edge_te"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Missing test for updated port_order_types parameter.
The changes to the 'port_order_types' parameter in 'edge_coupler_silicon' and 'edge_coupler_silicon_2' should be covered by unit tests to ensure that the new naming convention is correctly implemented and does not affect other functionalities.
@@ -259,7 +259,7 @@ def grating_coupler_elliptical( | |||
width=10, | |||
orientation=0, | |||
layer=layer, | |||
port_type="optical", | |||
port_type=f"optical_{polarization}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Missing test for dynamic port_type based on polarization.
The introduction of a dynamic 'port_type' based on the 'polarization' parameter should be validated through tests to ensure that it behaves as expected across different inputs.
port_type=f"optical_{polarization}", | |
import pytest | |
@pytest.mark.parametrize("polarization", ["TE", "TM", "random"]) | |
def test_port_type(polarization): | |
expected_port_type = f"optical_{polarization}" | |
# Assuming a function `create_component` that uses the `port_type` logic | |
component = create_component(polarization=polarization) | |
assert component.port_type == expected_port_type |
@@ -141,7 +141,7 @@ def grating_coupler_rectangular( | |||
xport = np.round((x0 + cgrating.x) / 2, 3) | |||
c.add_port( | |||
name="o2", | |||
port_type="optical", | |||
port_type=f"vertical_{polarization}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Ensure consistency in port naming convention.
The test should verify that the new 'vertical_{polarization}' naming is consistent across all components and does not introduce discrepancies.
port_type=f"vertical_{polarization}", | |
import pytest | |
def test_vertical_polarization_naming_consistency(): | |
# Assuming `get_port_type` is a method that retrieves the port type of a component | |
# and `expected_polarization` is the polarization we expect to be part of the port type name. | |
component = create_grating_coupler() # Function to create a grating coupler component | |
port_type = component.get_port_type() | |
expected_polarization = "vertical_TE" # Example polarization, adjust as necessary | |
assert expected_polarization in port_type, f"Port type '{port_type}' does not include expected polarization '{expected_polarization}'" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2700 +/- ##
=======================================
Coverage 71.73% 71.73%
=======================================
Files 366 366
Lines 23784 23784
Branches 3876 3876
=======================================
Hits 17061 17061
Misses 5593 5593
Partials 1130 1130 ☔ View full report in Codecov by Sentry. |
edge and vertical ports should be called differently than
optical
this PR changes them to
vertical_{polarization}
@tvt173