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

Ping tutorial updates #476

Merged
merged 13 commits into from
Oct 31, 2024
Merged

Ping tutorial updates #476

merged 13 commits into from
Oct 31, 2024

Conversation

agchesebro
Copy link
Member

General PR to address the comments from 29 and 26 over in the docs.

Also once we're happy with this we should close 16. Forgot to ping about that (pun intended) during the last pass.

Annabel is working on the updated graphic and should be done this week, so I'm going to try and wrap up all the other edits by then too.

@agchesebro
Copy link
Member Author

This branch should now address all of the direct tutorial comments over in 29. Still need to adjust per 26, especially specific comments 1, 2, 3 and minor suggestions 1, 2, 4. No more code changes though - just cosmetics and text to improve UX.

Still missing API links though - will discuss further before merging.
@agchesebro
Copy link
Member Author

@harisorgn I think this is ready for review. I've taken a stab at all the edits Scott suggested. The biggest outstanding thing that I see is there are no direct links to the API or other documentation, but I've added docstrings and included instructions on querying them from the REPL to fill that gap for now.

@agchesebro
Copy link
Member Author

Also unsure why the documentation fails to build, but all the other tests pass 🙃

@harisorgn
Copy link
Member

The biggest outstanding thing that I see is there are no direct links to the API or other documentation

I'll fix the API page and ping people to add docstrings where missing. Thanks for adding yours.

Also unsure why the documentation fails to build, but all the other tests pass 🙃

I think you forgot to comment out the inner for loops properly on the block that fails (shown on the docs build output).

@agchesebro
Copy link
Member Author

Hmm maybe it can't handle a block that's all commented? Because the inner for loops have the same as the outer ones (## to start the lines). Using Literate locally I see that the whole block is commented out too, so nothing should be running there 🤔.

@harisorgn
Copy link
Member

Sorry, errors during docs build point to the relevant block in the md file ofc, not the jl, so I thought you missed one # in all inner for loop lines.

@agchesebro
Copy link
Member Author

No worries I thought I did too for a minute haha. It looks like including a block with just commented code is the issue, so I'll just remove the optional code for now and we can cover adjacency matrices in another tutorial.

@harisorgn
Copy link
Member

Let me try one thing

@agchesebro
Copy link
Member Author

Oh nice that looks like it worked!

@harisorgn harisorgn merged commit 3f04394 into master Oct 31, 2024
6 checks passed
@harisorgn harisorgn deleted the ping-tutorial-updates branch October 31, 2024 19:35
david-hofmann pushed a commit that referenced this pull request Nov 11, 2024
* Add citation links

* Add alternative method for creating edges from adjacency matrix

* New PING network illustration

Thanks to Annabel for creating a much cleaner version!

* Final tweaks to address direct comments

* Updating documentation for PING blocks

Missing docstrings for API creation and Scott's right we should have that for tutorials

* Adding bullet points to the intro

* Suggestions are pretty much covered now

Still missing API links though - will discuss further before merging.

* Just seeing if this compiles the docs

* and does this compile

* try adding a note

* add offset to last line

---------

Co-authored-by: haris organtzidis <[email protected]>
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.

Create PING network tutorial
2 participants