Skip to content
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

Forbid width missmatch #2701

Merged
merged 5 commits into from Apr 25, 2024
Merged

Forbid width missmatch #2701

merged 5 commits into from Apr 25, 2024

Conversation

joamatab
Copy link
Contributor

  • raise error by default if there are any width, layer or port_type missmatch. Inspired by kfactory

@nikosavola
@sebastian-goeldi
@tvt173

@joamatab joamatab added the enhancement New feature or request label Apr 24, 2024
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

"e3",
bottom_heater_ref.ports["o1"],
allow_layer_mismatch=True,
allow_type_mismatch=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (performance): Assess the impact of type mismatch allowances on system performance.

Allowing type mismatches can sometimes lead to performance degradation or unexpected behavior. It's crucial to evaluate whether the benefits outweigh the potential risks in this context.

Suggested change
allow_type_mismatch=True,
allow_type_mismatch=False,

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 90.74074% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 71.71%. Comparing base (5a3d9f6) to head (422cb9b).
Report is 2 commits behind head on main.

Files Patch % Lines
gdsfactory/component_reference.py 40.00% 3 Missing ⚠️
gdsfactory/components/via_cutback.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2701      +/-   ##
==========================================
- Coverage   71.73%   71.71%   -0.02%     
==========================================
  Files         366      366              
  Lines       23784    23795      +11     
  Branches     3876     3876              
==========================================
+ Hits        17061    17065       +4     
- Misses       5593     5603      +10     
+ Partials     1130     1127       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvt173
Copy link
Collaborator

tvt173 commented Apr 24, 2024

in some cases this may be nice, in some cases this may be undesirable. i.e. you want to see in the GDS where the error is happening. is there a way to globally disable this if necessary?

@joamatab
Copy link
Contributor Author

yes, you can define a .env file to ignore or warn, now it's showing you an error

https://gdsfactory.github.io/gdsfactory/notebooks/12_config.html

@joamatab joamatab merged commit add7f60 into main Apr 25, 2024
13 checks passed
@joamatab joamatab deleted the forbid_width_missmatch branch April 25, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants