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

chore: port entire Part 1 / Intro to Python #245

Merged
merged 15 commits into from
Aug 19, 2024
Merged

Conversation

Saransh-cpp
Copy link
Member

XRef #238
Also closes #201

  • ported basics + variables + functions from doctoral school
  • "defining functions" notebook's name started from 04 for some reason, putting it way down in the table of contents. Now it is 16 and calling functions is 17

@Saransh-cpp Saransh-cpp marked this pull request as draft August 14, 2024 13:29
@Saransh-cpp
Copy link
Member Author

I have been going through more material and the content is not only more descriptive, but also arranged well. There is no instance of information being present in RSD course but absent from doctoral training course. I will port all the pages together so that no content is skipped in between PRs (given that the content will be rearranged a bit).

@Saransh-cpp Saransh-cpp force-pushed the saransh/port-doctoral-1 branch from f3a76a6 to 81d7e3c Compare August 14, 2024 16:13
@Saransh-cpp Saransh-cpp changed the title chore: port basics + variables + functions from doctoral school chore: port entire Part 1 / Intro to Python Aug 15, 2024
@Saransh-cpp Saransh-cpp marked this pull request as ready for review August 15, 2024 10:22
Copy link
Member

@dpshelio dpshelio left a comment

Choose a reason for hiding this comment

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

Since this is a long PR, I'm going to review file by file. Here some suggestions for 016functions

ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
Comment on lines 86 to 87
# This is pretty awful style, in general, functions should normally be side-effect free.
#
# Here is a contrived example of a function that makes plausible use of a side-effect
#
Copy link
Member

Choose a reason for hiding this comment

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

I'll probably leave that as to warn about the following example. But it needs some explanation around it like:

# Note that the function below we are using `[:]`. This is used to update the content of the list, and though this function is not returning anything, it's changing the elements of the list.

ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
Comment on lines 124 to 125
# ## Early Return

Copy link
Member

Choose a reason for hiding this comment

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

This needs a comment about it. Could you at least write a comment with a TODO?

ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
Copy link
Member

@dpshelio dpshelio left a comment

Choose a reason for hiding this comment

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

second file reviewed!

ch01python/00pythons.ipynb.py Outdated Show resolved Hide resolved
ch01python/00pythons.ipynb.py Outdated Show resolved Hide resolved
ch01python/00pythons.ipynb.py Show resolved Hide resolved
ch01python/00pythons.ipynb.py Outdated Show resolved Hide resolved
ch01python/00pythons.ipynb.py Outdated Show resolved Hide resolved
@dpshelio
Copy link
Member

It was easier to review and push the other files. I let you "fix" the first two ;)

ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
ch01python/016functions.ipynb.py Outdated Show resolved Hide resolved
Saransh-cpp and others added 2 commits August 19, 2024 11:12
Co-authored-by: David Pérez-Suárez <d.perez-suarez@ucl.ac.uk>
@Saransh-cpp
Copy link
Member Author

Thanks for the review and fixing my mess, @dpshelio! 🔨

I have updated the PR with your suggestions.

@Saransh-cpp Saransh-cpp requested a review from dpshelio August 19, 2024 10:39
Copy link
Member

@dpshelio dpshelio left a comment

Choose a reason for hiding this comment

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

Awesome!!! You've not messed anything 😉 . I'm going to add the small suggestions and merge it now. 🙌

ch01python/00pythons.ipynb.py Outdated Show resolved Hide resolved
ch01python/00pythons.ipynb.py Show resolved Hide resolved
ch01python/00pythons.ipynb.py Show resolved Hide resolved
ch01python/00pythons.ipynb.py Show resolved Hide resolved
ch01python/00pythons.ipynb.py Outdated Show resolved Hide resolved
@dpshelio dpshelio enabled auto-merge August 19, 2024 12:16
@dpshelio dpshelio merged commit 2eac01a into main Aug 19, 2024
5 checks passed
@Saransh-cpp Saransh-cpp deleted the saransh/port-doctoral-1 branch August 19, 2024 12:44
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.

Add explanations about why and when notebooks are (not) good
2 participants