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

Notebook on Lowest Common Ancestor #69

Merged
merged 22 commits into from
Mar 31, 2023

Conversation

dtekinoglu
Copy link
Contributor

@dtekinoglu dtekinoglu commented Apr 19, 2022

No description provided.

@dtekinoglu dtekinoglu changed the title Create LCA Notebook Notebook on Lowest Common Ancestor Apr 19, 2022
@dtekinoglu
Copy link
Contributor Author

Hello @MridulS , I am continuously getting the following error:

make: *** [Makefile:20: html] Error 1

Do you have any idea about what could be the reason?

@inomag
Copy link

inomag commented Apr 25, 2022

@dtekinoglu You need to include the content in nx-guides/content/algorithms/index.md

@dtekinoglu
Copy link
Contributor Author

Thank you! @inomag

@inomag
Copy link

inomag commented Apr 25, 2022

Hi @dtekinoglu, I am not sure what is wrong here but I think you can make the following changes -

  • Instead of using images in base64 form, you can directly save the static images in a images folder in the same location and use them (Not the ones generated by code)
  • You have written !pip install pydot in the code, which might cause some problems. Include the necessary dependencies in requirements.txt

@MridulS
Copy link
Member

MridulS commented Apr 25, 2022

@dtekinoglu Hmm there are some weird markdown cells here.
Can you try running this on your notebook?

$ jupytext notebook.ipynb --to myst

and then push it here?

Thanks!

@dtekinoglu
Copy link
Contributor Author

Thank you @MridulS it passes the checks now 😄 I initally included a quiz cell which requires user input, but I thought that it is not supported. Thus, I removed that part. Am I correct? Do you suggest me to add it back?

@MridulS
Copy link
Member

MridulS commented Apr 25, 2022

I initally included a quiz cell which requires user input, but I thought that it is not supported. Thus, I removed that part. Am I correct?

Yeah we are not that fancy just yet! For now let's just keep narrative notebooks for our users :)

@MridulS
Copy link
Member

MridulS commented Apr 25, 2022

You can check the doc build here https://output.circle-artifacts.com/output/job/427d7bcf-0a12-412e-af58-618e4985f1e2/artifacts/0/site/_build/html/content/algorithms/lca/LCA.html

Please do checkout the suggestions by @inomag, they should help with cleaning up the output :)

@dtekinoglu dtekinoglu marked this pull request as ready for review July 18, 2022 19:11
@dtekinoglu
Copy link
Contributor Author

@MridulS how can I apply lint pre-commit changes for the notebooks?

@MridulS
Copy link
Member

MridulS commented Jul 21, 2022

@dtekinoglu I pushed the changes required to make the linters happy. It needs to run the pre-commit linters on the jupyter notebook (.ipynb) before converting it to myst.

# The following commands finds ALL the .md files in the repository
$ find content/ -name "*.md" -exec jupytext --to notebook {} \;
# Run pre-commit on all files
$ pre-commit run --all-files --show-diff-on-failure --color always
# Convert all the new "cleaned" ipynb files to myst
$ find content/ -name "*.ipynb" -exec jupytext --to md:myst {} \;
# Remove the .ipynb files
$ find content/ -name "*.ipynb" -exec rm {} \;

After this you should see the changes the linters made inside the markdown myst files. I know this isn't an ideal workflow but currently nbqa doesn't natively support myst format.

```{code-cell}
import matplotlib.pyplot as plt
import networkx as nx
from networkx.drawing.nx_pydot import graphviz_layout
Copy link
Member

Choose a reason for hiding this comment

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

Just a quick note: Could you use from networkx.drawing.nx_agraph import graphviz_layout instead of nx_pydot? We would like to deprecate pydot :)

@MridulS
Copy link
Member

MridulS commented Mar 31, 2023

@dtekinoglu well finally did the review :) I'll go ahead and merge this. Thanks for your contributions :)

@MridulS MridulS merged commit acde9a2 into networkx:main Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants