-
Notifications
You must be signed in to change notification settings - Fork 6
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
cspdk cleanup #51
cspdk cleanup #51
Conversation
🧙 Sourcery has finished reviewing your pull request! Tips
|
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.
We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.
ret = {} | ||
for k, v in net.get("instances", {}).items(): | ||
ret[k] = { | ||
"component": v.get("component", ""), | ||
"settings": v.get("settings", {}), | ||
} | ||
return ret |
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.
issue (code-quality): We've found these issues:
- Convert for loop into dictionary comprehension (
dict-comprehension
) - Inline variable that is immediately returned (
inline-immediately-returned-variable
)
ret = {} | ||
for k, v in net.get("instances", {}).items(): | ||
ret[k] = { | ||
"component": v.get("component", ""), | ||
"settings": v.get("settings", {}), | ||
} | ||
return ret |
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.
issue (code-quality): We've found these issues:
- Convert for loop into dictionary comprehension (
dict-comprehension
) - Inline variable that is immediately returned (
inline-immediately-returned-variable
)
ret = {} | ||
for k, v in net.get("instances", {}).items(): | ||
ret[k] = { | ||
"component": v.get("component", ""), | ||
"settings": v.get("settings", {}), | ||
} | ||
return ret |
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.
issue (code-quality): We've found these issues:
- Convert for loop into dictionary comprehension (
dict-comprehension
) - Inline variable that is immediately returned (
inline-immediately-returned-variable
)
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.
looks good to me
cspdk/sin300/tech.py
Outdated
PAD: Layer = (41, 0) | ||
NITRIDE: Layer = (203, 0) | ||
NITRIDE_ETCH: Layer = (204, 0) | ||
# TODO: how can we make this pass type checking? |
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.
remove?
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.
no, that makes pyright error out (not a huge deal, but I like my repositories type clean)
Hi @joamatab , I think everything is now fixed. I went through the yaml definition differences and decided the differences are fine, so I regenerated the yaml test references. I also went through the gds files and for the most part the differences where fine/to be expected as well. However, there is a minor issue for grating_coupler_rectangular where there seems to be extra padding around the grating in the old version which now seems to be gone in the new version. Can you figure out why? Or if its an issue? |
Some more cleanup of cspdk.
passes all tests except regression tests (note: you'll need the current master branch of gdsfactory!)
Please review and let me know what you think @joamatab