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

introduction update #460

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

serynabatov
Copy link
Contributor

@serynabatov serynabatov commented Apr 14, 2024

Hi!
I'm a collegue of https://github.com/himahuja. Unfortunatelly I won't be able to participate at the call
But he has told me everything.

I will going to provide my reasoning of why I have changed these lines of description.

Plus further I'll create a few different PRs (for the easier review purposes)

@serynabatov serynabatov requested a review from a team as a code owner April 14, 2024 17:01
@serynabatov serynabatov requested review from mgrover1 and jukent and removed request for a team April 14, 2024 17:01
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

github-actions bot commented Apr 14, 2024

👋 Thanks for opening this PR! The Cookbook will be automatically built with GitHub Actions. To see the status of your deployment, click below.
🔍 Git commit SHA: ec471de
✅ Deployment Preview URL: https://ProjectPythia.github.io/pythia-foundations/_preview/460

@serynabatov
Copy link
Contributor Author

I have left the comments
Please comment me back if you think that something is not applicable
Or I should revert it
In the meanwhile I'm going to add the new PRs

Have a nice day!

@jukent
Copy link
Contributor

jukent commented Apr 15, 2024

Thanks for this @serynabatov! I just added a couple edits.

@serynabatov
Copy link
Contributor Author

Hi @jukent !

I haven't found any comments unfortunately
Have you submitted the review?

foundations/quickstart.ipynb Outdated Show resolved Hide resolved
foundations/quickstart.ipynb Outdated Show resolved Hide resolved
foundations/quickstart.ipynb Outdated Show resolved Hide resolved
@jukent
Copy link
Contributor

jukent commented Apr 20, 2024

Hi @jukent !

I haven't found any comments unfortunately
Have you submitted the review?

Oops! Good catch.

@serynabatov
Copy link
Contributor Author

@jukent
Quick question
Does Pythia have some de facto standard to commit the suggestions?
I've read them all, they make sense to me

What I mean - should I apply them locally and then update the PR or I can just say to Github "Ok, commit" and you will receive 5 different commits in this branch right now

(The second one won't be a problem though if Pythia squashes the commits)

@jukent
Copy link
Contributor

jukent commented Apr 20, 2024

@jukent
Quick question
Does Pythia have some de facto standard to commit the suggestions?
I've read them all, they make sense to me

What I mean - should I apply them locally and then update the PR or I can just say to Github "Ok, commit" and you will receive 5 different commits in this branch right now

(The second one won't be a problem though if Pythia squashes the commits)

I usually do the 2nd option but it's up to you!

@serynabatov
Copy link
Contributor Author

Ok, I've applied the comments
I'll check the pipelines (where I've been wrong, and then I'll ping you)
In the meanwhile I am creating a second PR with the numpy exercises

@serynabatov serynabatov requested a review from jukent April 20, 2024 14:42
@@ -54,6 +54,35 @@
"print(\"Hello interweb\")"
Copy link
Member

@brian-rose brian-rose May 8, 2024

Choose a reason for hiding this comment

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

This section is a nice addition! Here's my suggested language tweak of the first paragraph:

Variables are containers for storing data, whether simple or complex. They can represent primitive data types or point to other more sophisticated containers, e.g., lists and dicts (which we will introduce later in this notebook).


Reply via ReviewNB

@@ -453,159 +482,6 @@
" print(\"The value is:\", mypet[key])"
]
},
{
Copy link
Member

Choose a reason for hiding this comment

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

What is the argument for removing the numpy and matplotlib sections here?

This notebook is meant to be a "quick tour" of Python usage particularly for people who are arriving from other programming languages. These sections are meant to introduce the modularity of the Python language and the idea of importing specialized packages. They also give a brief flavor of two of the most fundamental packages for scientific python.

What about keeping these sections, but just adding a note to help guide beginners?

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.

3 participants