-
Notifications
You must be signed in to change notification settings - Fork 41
add ports for tests #495
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
base: main
Are you sure you want to change the base?
add ports for tests #495
Conversation
Reviewer's GuideAdds a new regression test to validate optical port placement against drawn geometry for all parametrized cells, and updates repository ownership metadata in CODEOWNERS. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 - I've left some high level feedback:
- The conversion from edge length to microns uses a hard-coded
0.001factor; consider computing the physical length usingdbu_in_um(e.g.edge_length_um = port_edge.length() * dbu_in_um) to avoid assuming a specific database unit. - Within
test_optical_port_positions, you are manually raisingAssertionErrorinstead of using plainassertstatements; switching toassertwill make the tests more idiomatic and produce clearer diffs/output in pytest.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The conversion from edge length to microns uses a hard-coded `0.001` factor; consider computing the physical length using `dbu_in_um` (e.g. `edge_length_um = port_edge.length() * dbu_in_um`) to avoid assuming a specific database unit.
- Within `test_optical_port_positions`, you are manually raising `AssertionError` instead of using plain `assert` statements; switching to `assert` will make the tests more idiomatic and produce clearer diffs/output in pytest.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Going to leave these alone until tests pass and ports are actually fixed |
ThomasPluck
left a comment
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.
Fix ports
Summary by Sourcery
Add validation for optical port geometry in si220 cband tests and update repository code ownership configuration.
Tests:
Chores: