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

attempt to add interface models #14

Closed
wants to merge 2 commits into from

Conversation

simbilod
Copy link
Contributor

@simbilod simbilod commented Jan 6, 2023

Hi @flaport , I've started giving a shot at #13 , see attached branch. Can I merge to a dev branch here maybe?

Couple questions:

It's my first time using nbdev for development, what is your typical workflow? I've been adding cells in the most relevant notebook and rerunning the higher cells with code I need to change, but it seems a bit unwieldy. Also what debugger do you use?

Playing with notebook 07, I was able to update the netlist/add models conditioned on if a key of the new "connection_models" dict matches an existing connection. The simplest plan would be to replace, in the unrolled netlist, all the connections that are represented in the "connection_models" keys by a old_instance --> dummy_component associated with connection_model --> old_instance sequence, and then running the calculation as usual.

But now I am getting errors about the instance being added not being a gdsfactory Component. However I think we don't think gdsfactory is a dependency. Or is it? What do you recommend?

Thanks!

@simbilod simbilod marked this pull request as draft January 6, 2023 01:13
clean notebook outputs
@simbilod
Copy link
Contributor Author

simbilod commented Jan 6, 2023

It also looks like the notebooks did not cleanup properly. Or maybe they're just hard to read haha

@@ -599,6 +738,11 @@
" netlist, instance_models = _extract_instance_models(netlist)\n",
" \n",
" recnet: RecursiveNetlist = _validate_net(netlist)\n",
"\n",
" # add connection models into recursive netlist\n",
" if connection_models:\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the call to modify the netlist/models based on the contents of connection_models

" return connections, ports, models\n",
"\n",
"\n",
"def _add_connection_models_recursive(recnet, models, connection_models):\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function and the one below try to add the connection_models to the main netlist/models

@flaport
Copy link
Owner

flaport commented Jan 7, 2023

Hi Simon,

I realize using nbdev might be a bit of a strange environment to work in. I personally love it, but I think to encourage future contributions I might convert the repository to a more standard layout again.

Personally, I often just use the interactive nature of a notebook for debugging, but there are other options such as this one:
https://jupyterlab.readthedocs.io/en/stable/user/debugger.html

In terms of generating the library from the notebooks, it's easiest to do a pre-commit install, which will make sure the generated library is always in sync with the notebooks once you commit.

I'm fine with merging into main directly. As long as a commit is not tagged I consider this a dev branch.

Is the PR finished? Currently when I run 07_circuit.ipynb it errors out in the 'Attempt with notebook 5 example' section...

@simbilod
Copy link
Contributor Author

Thanks for the tips!

I still need to figure out this component issue, might not be able to get to it for another week or so

@simbilod
Copy link
Contributor Author

simbilod commented May 4, 2023

Closing for now since this can be emulated with interface components anyway

@simbilod simbilod closed this May 4, 2023
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.

2 participants