Add inheritance for bounding box operations #575
Conversation
Reviewer's Guide by SourceryThis pull request introduces inheritance for bounding box operations on instances, providing a more intuitive and consistent API for transformations. It also includes new tests to verify the correctness of the implementation. Sequence diagram for flexgrid bounding box operationssequenceDiagram
participant F as Flexgrid
participant I as Instance
participant B as BoundingBox
F->>I: Get bbox/dbbox
I->>B: Create bounding box
B-->>I: Return bounds
I-->>F: Return bbox
F->>I: Apply transformation
Note over F,I: Transform instance position<br/>based on alignment and spacing
F->>I: Get updated bbox
I-->>F: Return new bounds
Class diagram for bounding box operations inheritanceclassDiagram
class Instance {
+bbox()
+dbbox()
+trans
+dcplx_trans
}
class BoundingBox {
+left
+right
+top
+bottom
+center()
}
Instance --> BoundingBox : returns
note for Instance "Handles both integer and floating-point transformations"
note for BoundingBox "Represents geometric bounds"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
| if port.base.trans: | ||
| self.shapes(port.layer).insert(poly.transformed(port.trans)) | ||
| self.shapes(port.layer).insert( | ||
| kdb.Text(port.name if port.name else "", port.trans) |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| kdb.Text(port.name if port.name else "", port.trans) | |
| kdb.Text(port.name or "", port.trans) |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
| else: | ||
| self.shapes(port.layer).insert(poly, port.dcplx_trans) | ||
| self.shapes(port.layer).insert( | ||
| kdb.Text(port.name if port.name else "", port.trans) |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| kdb.Text(port.name if port.name else "", port.trans) | |
| kdb.Text(port.name or "", port.trans) |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
src/kfactory/kcell.py
Outdated
| if isinstance(cross_section, SymmetricalCrossSection): | ||
| if cross_section.enclosure != self.kcl.get_enclosure( | ||
| cross_section.enclosure | ||
| ): | ||
| return self.get_cross_section( | ||
| CrossSectionSpec( | ||
| sections=cross_section.enclosure.model_dump()["sections"], | ||
| main_layer=cross_section.main_layer, | ||
| name=cross_section.name, | ||
| width=cross_section.width, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if isinstance(cross_section, SymmetricalCrossSection): | |
| if cross_section.enclosure != self.kcl.get_enclosure( | |
| cross_section.enclosure | |
| ): | |
| return self.get_cross_section( | |
| CrossSectionSpec( | |
| sections=cross_section.enclosure.model_dump()["sections"], | |
| main_layer=cross_section.main_layer, | |
| name=cross_section.name, | |
| width=cross_section.width, | |
| ) | |
| ) | |
| if isinstance(cross_section, SymmetricalCrossSection) and cross_section.enclosure != self.kcl.get_enclosure( | |
| cross_section.enclosure | |
| ): | |
| return self.get_cross_section( | |
| CrossSectionSpec( | |
| sections=cross_section.enclosure.model_dump()["sections"], | |
| main_layer=cross_section.main_layer, | |
| name=cross_section.name, | |
| width=cross_section.width, | |
| ) | |
| ) | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| _infos = infos() if infos else LayerInfos() | ||
| super().__init__( | ||
| name=name, | ||
| layer_enclosures=LayerEnclosureModel(dict()), |
There was a problem hiding this comment.
suggestion (code-quality): Replace dict() with {} (dict-literal)
| layer_enclosures=LayerEnclosureModel(dict()), | |
| layer_enclosures=LayerEnclosureModel({}), |
Explanation
The most concise and Pythonic way to create a dictionary is to use the{}notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
x = {"first": "thing"}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 = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
Similar reasoning and performance results hold for replacing list() with [].
| if name is None: | ||
| if main_layer is not None and main_layer.name != "": | ||
| name = main_layer.name |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if name is None: | |
| if main_layer is not None and main_layer.name != "": | |
| name = main_layer.name | |
| if name is None and (main_layer is not None and main_layer.name != ""): | |
| name = main_layer.name | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| options.cell_conflict_resolution | ||
| != kdb.LoadLayoutOptions.CellConflictResolution.RenameCell | ||
| ): | ||
| self.kcl.set_meta_data() |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into method [×2] (
extract-method) - Use f-string instead of string concatenation (
use-fstring-for-concatenation)
| if isinstance(inst, Instance): | ||
| return Instance(self.kcl, self._base_kcell.kdb_cell.insert(inst.instance)) | ||
| else: | ||
| if not property_id: | ||
| return Instance(self.kcl, self._base_kcell.kdb_cell.insert(inst)) | ||
| else: | ||
| assert isinstance(inst, kdb.CellInstArray | kdb.DCellInstArray) | ||
| return Instance( | ||
| self.kcl, self._base_kcell.kdb_cell.insert(inst, property_id) | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Remove unnecessary else after guard condition (
remove-unnecessary-else) - Swap if/else branches [×2] (
swap-if-else-branches)
| ) | ||
| if self.locked: | ||
| raise LockedError(self) | ||
| if trans: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Remove unnecessary else after guard condition (
remove-unnecessary-else) - Hoist for/while loops out of nested conditionals (
hoist-loop-from-if)
| """ | ||
| self.clear_meta_info() | ||
| if not self.is_library_cell(): | ||
| for i, port in enumerate(self.ports): |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Hoist repeated code outside conditional statement (
hoist-statement-from-if) - Use named expression to simplify assignment and conditional [×3] (
use-named-expression)
| ) | ||
| data["library"].register(data["name"]) | ||
| return data | ||
| def get_meta_data( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract duplicate code into method (
extract-duplicate-method) - Low code quality found in ProtoTKCell.get_meta_data - 15% (
low-code-quality)
Explanation
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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
+ Coverage 57.76% 59.21% +1.45%
==========================================
Files 49 49
Lines 9529 9163 -366
Branches 1764 1739 -25
==========================================
- Hits 5504 5426 -78
+ Misses 3486 3203 -283
+ Partials 539 534 -5 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
Add support for mirroring, moving, and rotating instances using both database and layout coordinates. Update the flexgrid and layer_cat functions to use layout coordinates when performing bounding box operations. Add tests for instance operations and attributes, as well as update existing tests to accommodate these changes.
New Features:
Tests: