Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new experimental data #23

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Added new experimental data #23

wants to merge 23 commits into from

Conversation

hung-chi
Copy link

@hung-chi hung-chi commented Nov 1, 2022

  • Tests pass
  • Appropriate changes to README are included in PR

@msaligane
Copy link
Collaborator

@hung-chi Can you add the notebook you showed me last Friday?

@hung-chi
Copy link
Author

hung-chi commented Nov 1, 2022

@hung-chi Can you add the notebook you showed me last Friday?

You can find the notebook at https://github.com/hung-chi/skywater-pdk-sky130-raw-data/blob/develop/notebooks/electrical_characterization_sky130_epfl.ipynb

@mithro
Copy link
Contributor

mithro commented Nov 1, 2022

@hung-chi - Is that notebooks part of this pull request?

@mithro mithro requested a review from QuantamHD November 1, 2022 15:38
@hung-chi
Copy link
Author

hung-chi commented Nov 1, 2022

@hung-chi - Is that notebooks part of this pull request?

Yes, the notebook is included. You could find it in my forked repo at https://github.com/hung-chi/skywater-pdk-sky130-raw-data/blob/develop/notebooks/electrical_characterization_sky130_epfl.ipynb

@hung-chi
Copy link
Author

hung-chi commented Nov 8, 2022

@msaligane

I added the Gm/ID plot. You can find them in the updated notebook.

@hung-chi
Copy link
Author

hung-chi commented Dec 5, 2022

#23

Copy link
Member

@proppy proppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late feedback!

Just a few minor cosmetic changes.

README.rst Outdated
@@ -10,6 +10,9 @@ Experimental Data

The experimental data in this repository is currently stored in the `.mdm` file format.

**Other data**

* (2022-Nov-1) New experimental data by EPFL is stored in the `.csv` format at `sky130_fd_pr/epfl <./sky130_fd_pr/epfl>`_. The corresponded notebook can be found at `notebooks/electrical_characterization_sky130_epfl.ipynb <./notebooks/electrical_characterization_sky130_epfl.ipynb>`_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move the data under a different top level dirrectory for consistency?

sky130_fd_pr_epfl/cells

@mithro any input there?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in be9ac42

@@ -0,0 +1,1204 @@
{
"cells": [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a new cell with a colab badge:

+  {
+   "cell_type": "markdown",
+   "metadata": {
+    "id": "view-in-github",
+    "colab_type": "text"
+   },
+   "source": [
+    "<a href=\"https://colab.research.google.com/github/proppy/skywater-pdk-sky130-raw-data/blob/develop/notebooks/electrical_characterization_sky130_epfl.ipynb\" target=\"_parent\"><img src=\"https://colab.research.google.com/assets/colab-badge.svg\" alt=\"Open In Colab\"/></a>"
+   ]
+  },

and another one that clone the repo, similar to what we have for the other notebooks:
https://github.com/google/skywater-pdk-sky130-raw-data/blob/main/notebooks/sky130_plot_gm.ipynb
https://github.com/google/skywater-pdk-sky130-raw-data/blob/main/notebooks/sky130_plot_current.ipynb

also maybe rename it to have s similar naming scheme as the other one?

@bmurmann should we look into consolidating with your existing notebook and allow it to work across multiple dataset?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My notebook is very simple, best to build on this new one instead of consolidating/merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Notebooks were renamed in 04d30b1.
Also, README was updated correspondingly in f6bc6ab

@msaligane msaligane requested review from bmurmann and removed request for bmurmann February 15, 2023 23:53
@proppy
Copy link
Member

proppy commented Feb 17, 2023

@hung-chi thanks for working on this!

Do you think it would it makes sense to modify the notebook so that they can be opened in https://colab.research.google.com/ ?

If you agree, maybe we can do the follow-up changes (either in this PR or separatly):

@hung-chi
Copy link
Author

@hung-chi thanks for working on this!

Do you think it would it makes sense to modify the notebook so that they can be opened in https://colab.research.google.com/ ?

If you agree, maybe we can do the follow-up changes (either in this PR or separatly):

Sure, I can do that in the PR. Also fine with inlining the utils in sky130_plot_epfl_data_general notebook.

1. included in-line utils
2. included colab
3. included copyright and license
4. included git clone
@hung-chi
Copy link
Author

Hi @proppy

I have updated the notebook by including the self-clone, in-line utils, colab.
The notebook will work correctly after merging.

Thanks

}
},
"source": [
"<h1>Electrical characterization of SkyWater 130 nm<span class=\"tocSkip\"></span></h1>\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the colab badge here too?

"outputs": [],
"source": [
"import pandas as pd\n",
"import sekve\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may need a few %pip install command before you can import this.

"import re\n",
"import os\n",
"import os.path as op\n",
"import sekve\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may need a few %pip install command before you can import this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants