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

Tutorials: XGI in X minutes #415

Merged
merged 13 commits into from
Aug 21, 2023
Merged

Conversation

thomasrobiglio
Copy link
Collaborator

I plan on working on this PR related to #414 over the next two weeks.
(quick note: the texts rely heavily on chatGPT's joy of life, we can then decide to tone it down a bit 😄)

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jul 15, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.93% 🎉

Comparison is base (087cb61) 90.90% compared to head (9d26977) 91.84%.
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
+ Coverage   90.90%   91.84%   +0.93%     
==========================================
  Files          47       60      +13     
  Lines        3916     4255     +339     
==========================================
+ Hits         3560     3908     +348     
+ Misses        356      347       -9     

see 41 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomasrobiglio thomasrobiglio linked an issue Jul 15, 2023 that may be closed by this pull request
tutorials/XGI in 5 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 5 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 5 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 5 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 5 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 5 minutes.ipynb Show resolved Hide resolved
@thomasrobiglio
Copy link
Collaborator Author

Thank you @maximelucas I will address your comments in the next days while working on the 15 min tuto

tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
tutorials/XGI in 15 minutes.ipynb Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

As Leo mentioned somewhere, we should check that the Github Action testing the notebook will test the new ones (it might test only names "Tutorial_*").

Quickly look at it though, it though looks fine?

@thomasrobiglio thomasrobiglio changed the title Tutorials restructuring Tutorials: XGI in X minutes Aug 18, 2023
@thomasrobiglio thomasrobiglio marked this pull request as ready for review August 19, 2023 18:04
@thomasrobiglio
Copy link
Collaborator Author

Ok, the three tutorials are ready, I have already addressed the comments by @maximelucas. As I had mentioned on Zulip maybe we can merge this before releasing the new version and then proceed with the rest of the tutorial restructuring (that's why I have renamed the PR and unlinked #414) later.

@@ -0,0 +1,679 @@
{
Copy link
Collaborator

@maximelucas maximelucas Aug 21, 2023

Choose a reason for hiding this comment

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

Ref to XGI in 5 min still missing :)


Reply via ReviewNB

@@ -0,0 +1,679 @@
{
Copy link
Collaborator

@maximelucas maximelucas Aug 21, 2023

Choose a reason for hiding this comment

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

Remove ' for example" ', at the beginning of the sentence


Reply via ReviewNB

@@ -0,0 +1,679 @@
{
Copy link
Collaborator

@maximelucas maximelucas Aug 21, 2023

Choose a reason for hiding this comment

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

After this I would just say something like "For other hypergraph matrices such as Laplacians, see the linalg documentation" with a link.


Reply via ReviewNB

@@ -0,0 +1,679 @@
{
Copy link
Collaborator

@maximelucas maximelucas Aug 21, 2023

Choose a reason for hiding this comment

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

Before wrapping up (or earlier in the tuto), what do you think of just advertising the stats with 2 short examples, and the link to docs/tuto?

Like

  • H.nodes.filterby("degree", 2)
  • df = H.nodes.multi(["degree", "clustering_coefficient"]).aspandas()

Reply via ReviewNB

@thomasrobiglio
Copy link
Collaborator Author

thank you @maximelucas for the comments, everything should be good to go now from my side

@thomasrobiglio
Copy link
Collaborator Author

the only thing is that I am not entirely sure that the hyperlinks to the XGI in [...] minutes are all working, in case I will do a micro PR fixing that :)

@maximelucas
Copy link
Collaborator

Looks good, thanks Thomas!!

@thomasrobiglio thomasrobiglio merged commit 1c12aa7 into xgi-org:main Aug 21, 2023
18 checks passed
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