-
Notifications
You must be signed in to change notification settings - Fork 204
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
improve get_cross_sections #2714
Conversation
joamatab
commented
May 5, 2024
- get cross_section can also get cross_section factories
- fix trans_sc_nc second port
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: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
use_slab_port: if True, uses the port associated with the slab layer (layer_slab) | ||
for the second output port of the component. | ||
If False, the second port uses the same layer as the first port (layer_wg). |
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 adding more detail to the 'use_slab_port' parameter description.
It would be helpful to specify what the implications are for signal integrity or other operational characteristics when changing the port layer.
use_slab_port: if True, uses the port associated with the slab layer (layer_slab) | |
for the second output port of the component. | |
If False, the second port uses the same layer as the first port (layer_wg). | |
use_slab_port: if True, uses the port associated with the slab layer (layer_slab) | |
for the second output port of the component. This can affect the modal properties | |
of the signal, potentially altering its propagation characteristics due to the | |
different material or structural properties of the slab layer. | |
If False, the second port uses the same layer as the first port (layer_wg), | |
maintaining consistent signal characteristics across both ports. |
gdsfactory/cross_section.py
Outdated
if callable(t[1]) and not t[0].startswith("_"): | ||
try: | ||
r = signature( | ||
t[1] if not isinstance(t[1], partial) else t[1].func | ||
).return_annotation | ||
if r == CrossSection or ( | ||
isinstance(r, str) and r.endswith("CrossSection") | ||
): | ||
xs[t[0]] = t[1] | ||
except ValueError as e: |
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_refinement): Review error handling strategy for cross-section retrieval.
Consider whether logging a warning is sufficient for error handling in this context, or if these errors should be escalated or handled differently.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2714 +/- ##
=======================================
Coverage 71.72% 71.72%
=======================================
Files 366 366
Lines 23804 23814 +10
Branches 3878 3881 +3
=======================================
+ Hits 17073 17081 +8
- Misses 5604 5605 +1
- Partials 1127 1128 +1 ☔ View full report in Codecov by Sentry. |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>