make CORNERSTONE package support all the different pdks #29
Conversation
joamatab
commented
Apr 30, 2024
joamatab
commented
- proof of concept of dividing it into multiple pdks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @joamatab - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -2,11 +2,11 @@ | |||
| from gdsfactory.get_factories import get_cells | |||
| from gdsfactory.pdk import Pdk | |||
|
|
|||
| from cspdk import cells, config, tech | |||
There was a problem hiding this comment.
suggestion (testing): Missing tests for new module structure.
The PR introduces a new module structure under 'cspdk.si220'. It's crucial to add tests to verify that the new imports and module paths are correctly set up and functional.
| @@ -3,15 +3,14 @@ | |||
| import matplotlib.pyplot as plt | |||
| import sax | |||
|
|
|||
| import cspdk | |||
There was a problem hiding this comment.
suggestion (testing): Integration tests needed for sample scripts.
Changes in the sample scripts suggest modifications in how components are instantiated and routed. Integration tests should be added to ensure these scripts still perform as expected with the new module paths.
cspdk/si220/__init__.py
Outdated
| ) | ||
| _cross_sections = get_cross_sections(tech) | ||
| PDK = Pdk( | ||
| name="cornerstone", |
There was a problem hiding this comment.
this is no longer a good, distinctive pdk name. what if we make it mirror the package name, i.e. cspdk.si220?
There was a problem hiding this comment.
yes, fixed now, i named it cornerstone_si220, cornerstone_sin300 ... and so on
