models/{ngspice,xyce}/testing/README: should document how to interpret simulation result

[proppy]

Expected Behavior

https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/models/ngspice/testing and https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/models/xyce/testing READMEs should document how to interpret simulation result.

Actual Behavior

https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/models/ngspice/testing and https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/models/xyce/testing READMEs assume developers/contributors are familiar with the simulation output to be able to interpret if there is failure or regression.

[atorkmabrains]

Hi @proppy and @mkkassem Please check the documentation in here:
https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/blob/main/models/ngspice/testing/README.md

[atorkmabrains]

@proppy and @mkkassem Changes in the documentation has been finalized.

[atorkmabrains]

https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/blob/main/models/xyce/testing/README.md

[proppy]

@mkkassem @atorkmabrains I think you can easily create a separate pull request for the models changes this way:

# clone upstream repo
git clone https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr
cd globalfoundries-pdk-libs-gf180mcu_fd_pr
# add your fork remote
git remote add mabrains [email protected]:mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr.git
git fetch mabrains
# create feature branch
git checkout -b improve-models-docs
# checkout (only) models changes
git checkout mabrains/main models
# create a new collapsed commit for (only) models changes 
git commit -m 'models: improve documentation'
# push back to your fork
git push mabrains improve-models-docs
# create the pull request

[atorkmabrains]

@proppy I have discussed that with @mithro before. That can't be done. Primitive renaming is a lateral change that affects everything and if you take one part and leave the other parts of the PDK unchanged, it will break the PDK. And there is another issue, doing PR will not work as well as the amount of changes are massive. And github doesn't allow that. I have shown this to @mithro and @mkkassem. That's why I was asking you guys to get the primitives renaming PRs in first before we change anything.

[proppy]

@atorkmabrains I'm confused, I don't think the README changes to the models/ directory that we're discussing here are related to the cell renaming changes? So they should be PR-able and mergeable separatly.

[atorkmabrains]

@proppy The documentation will not be aligned with what you have in this repo. Even if we only take readme changes, if you use it as is it will be broken.

[atorkmabrains]

@proppy Also, looking at the commands above, this will take everything in models folder including the renaming part and most likely github PR will not allow it.

[proppy]

Even if we only take readme changes, if you use it as is it will be broken.

The renaming changes don't seems to affect the models, ex: https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/pull/48/files, so if you were checking-in the models change that include rename + docs in this PR, it wouldn't be broken, it would just be inconsistent with the rest of the PR until the other PR gets merged (simarly if you merge the standalone rename PRs first, it would also be inconsistent if you don't update the models at the same time).

[proppy]

What's important for those change is that we're able to run the tests for a given PR, so you need to merge the test either before (or with) the rename.

[proppy]

Basically if you:

1. take the instruction in models/{ngspice,xyce}/testing/README: should document how to interpret simulation result #69 (comment)
2. force push to Updating primitives names in models directory #33 branches (https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/primitives_names_part2)
3. mark it as fixing this issue

That should be enough to provide a coherent set of change to review and merge (that include both naming change and appropriate documentation update to evaluate it).

[atorkmabrains]

@proppy the renaming of the primitives is split over 18 PRs due to it's sheer amount of changes. The PR that you are referring to is just one out of 18. Please check with @mithro I have discussed this with him about 4 months ago.

[proppy]

@atorkmabrains, yes but that particular PR (#33) should be revieweable/mergeable independently of the remaining 17 as long we integrate it with the rest of the models change as shown in #69 (comment) and #69 (comment).

[atorkmabrains]

@FaragElsayed2 Please take a look here as well.

[FaragElsayed2]

@atorkmabrains I have updated all docs related to models and it is merged in efabless version.

[atorkmabrains]

@FaragElsayed2 Let's keep this issue open until it's added to Google here.