Refactor of current_mirror.py #367
Conversation
I have broken down the current_mirror function into multiple smaller functions as this will improve readability and re-usability of the smaller function. Also added docstrings to the functions to make them more clear to users.
harshkhandeparkar
left a comment
harshkhandeparkar
left a comment
There was a problem hiding this comment.
Left a few comments but this looks good otherwise
|
|
||
|
|
||
| #@cell | ||
| def create_interdigitized_fets(pdk: MappedPDK, device: str, numcols: int, with_dummy: bool, **kwargs) -> Component: |
There was a problem hiding this comment.
Is this pattern used elsewhere, in other circuits perhaps? If yes, then this function can be reused there as well.
There was a problem hiding this comment.
I do not know repo well enough right now, but yes if this pattern is being used elsewhere then it can be replaced
There was a problem hiding this comment.
That's fine. Let it be for now.
| raise ValueError(f"Invalid device type: {device}. Must be 'nfet' or 'pfet'.") | ||
|
|
||
|
|
||
| def route_interdigitized_fets(pdk: MappedPDK, interdigitized_fets: Component) -> Component: |
There was a problem hiding this comment.
It would be better to prefix the function name with an _ or __ since it is only used as a subfunction in this file.
There was a problem hiding this comment.
Yes, I agree with that. I'll make the changes.
| ) | ||
|
|
||
| return top_level | ||
| """An instantiable current mirror Component. |
There was a problem hiding this comment.
It seems some of the documentation was removed. Is there a reason why it was shortened?
There was a problem hiding this comment.
I thought this would be better but in hindsight I think the initial documentation is a better choice.
There was a problem hiding this comment.
More elaborate documentation never hurts.
| return component | ||
|
|
||
|
|
||
| def current_mirror( |
There was a problem hiding this comment.
Have you tested if this function still works the same? For a temporary check, you can instantiate a few current mirrors and compare the results to the older function and ensure they are the same.
However, it would be really good if you add a test in the CI. You can take a look at this file as an example.
2 participants
I have broken down the current_mirror function into multiple smaller functions as this will improve readability and re-usability of the smaller function. Also added docstrings to the functions to make them more clear to users.