Support instances in path-length extraction from gds #468
Conversation
nikosavola
commented
Aug 14, 2024
- Re-support instances for path length GDS analysis
- More correct typing for layer
|
🧙 Sourcery has finished reviewing your pull request! Tips
|
There was a problem hiding this comment.
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.
| @@ -236,7 +236,7 @@ def centerline_single_poly_2_ports(poly, under_sampling, port_list) -> np.ndarra | |||
|
|
|||
| def extract_paths( | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace
list()with[][×2] (list-literal) - Don't assign to builtin variable
slice(avoid-builtin-shadow) - Low code quality found in extract_paths - 2% (
low-code-quality)
Explanation
The most concise and Pythonic way to create a list is to use the[] notation.
This fits in with the way we create lists with elements, saving a bit of mental
energy that might be taken up with thinking about two different ways of creating
lists.
x = ["first", "second"]Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
$ python3 -m timeit "x = list()"
5000000 loops, best of 5: 63.3 nsec per loop
$ python3 -m timeit "x = []"
20000000 loops, best of 5: 15.8 nsec per loop
Similar reasoning and performance results hold for replacing dict() with {}.
The most concise and Pythonic way to create a list is to use the [] notation.
This fits in with the way we create lists with elements, saving a bit of mental
energy that might be taken up with thinking about two different ways of creating
lists.
x = ["first", "second"]Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
$ python3 -m timeit "x = list()"
5000000 loops, best of 5: 63.3 nsec per loop
$ python3 -m timeit "x = []"
20000000 loops, best of 5: 15.8 nsec per loop
Similar reasoning and performance results hold for replacing dict() with {}.
Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
There was a problem hiding this comment.
Confused about where this was used. Not in this patch at least.
There was a problem hiding this comment.
It looks like my initial review comment might have been a bit too general and not directly related to the specific changes in this patch. I apologize for any confusion caused.
To clarify, the issues I mentioned are general code quality improvements that could be applied to the codebase, but they might not be directly relevant to the specific changes in this pull request. If these issues are not present in the diff you provided, you can disregard them for now.
If you have any specific questions or need further clarification on any part of the review, please let me know! I'm here to help.