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

Updates for 2018 #22

Merged
merged 15 commits into from
Jan 31, 2018
Merged

Updates for 2018 #22

merged 15 commits into from
Jan 31, 2018

Conversation

lgpage
Copy link
Member

@lgpage lgpage commented Jan 29, 2018

closes #12, #19, #21, #23, #24

Logan Page added 7 commits January 29, 2018 13:51
@lgpage lgpage requested review from alchemyst and smroux January 29, 2018 13:53
Copy link
Contributor

@smroux smroux left a comment

Choose a reason for hiding this comment

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

Most changes appear to be cosmetic, removal of bulky/messy examples seems to be biggest change.

All looks good.

@@ -257,7 +251,7 @@
}
},
"source": [
"## Do's and Don'ts\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it remain "Do's and Don'ts"?

Copy link
Member

Choose a reason for hiding this comment

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

This is a contentious phrase. I recommend rewording.

Copy link
Member Author

Choose a reason for hiding this comment

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

This also causes the spell checking to bomb out. I'll try reword.

"- [Roles & Responsibilities](#Roles-&-Responsibilities)\n",
"- [Do's and Don'ts](#Do%27s-and-Don%27ts)\n",
"- [Do and Don't](#Do-and-Don%27t)\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same "Do's and Don'ts" comment

@@ -321,7 +321,7 @@
"source": [
"### Example - Experimental Data\n",
"\n",
"- Consider an experimental setup whereby air is forced through a rectangular channel. Inside this channel there are several obstacles (cylindrical pins) that obstruct the flow.\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Setup more correct than set-up?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, will change it back to "setup".

@@ -21,10 +21,11 @@ Numpy
PhD
Pieterse
placeholder
radians
Roux
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you get the honour of having your surname?

Copy link
Member

Choose a reason for hiding this comment

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

I assume because "page" is already in the standard word list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. Spell check does not like your surname but is happy with mine

@@ -575,32 +587,15 @@
"<img src=\"./figures/Interpolation_example_linear.svg\" alt=\"Linear Interpolation\" style=\"height: 400px;\"/>"
]
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider this example to be too bulky and messy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Having the additional quadratic method does not add anything useful other than a very backwards way of introducing a wrapper function which I don't think is critical for the students to know. I would rather move this to one of the weekly assignments.

Copy link
Member

@alchemyst alchemyst left a comment

Choose a reason for hiding this comment

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

I'm down with the actual changes, but I've made a few small suggestions in the comments.

@@ -21,10 +21,11 @@ Numpy
PhD
Pieterse
placeholder
radians
Roux
Copy link
Member

Choose a reason for hiding this comment

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

I assume because "page" is already in the standard word list.

@@ -757,7 +757,7 @@
"#6\n",
"figure()\n",
"M = zeros(U.shape, dtype='bool')\n",
"M[U.shape[0]/3:2*U.shape[0]/3,U.shape[1]/3:2*U.shape[1]/3] = True\n",
"M[U.shape[0]//3:2*U.shape[0]//3,U.shape[1]//3:2*U.shape[1]//3] = True\n",
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this could be made much easier to understand by assigning rows, cols = U.shape

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too worried about the source code of the demo notebooks, just as long as it runs. We only briefly demo the outputs in any case.

"for i in np.arange(0, 5, 1):\n",
" s0 = h_init[i]\n",
" v0 = v_init[i]\n",
"for i in np.arange(0, 5, 1): # iterate over array indexes\n",
Copy link
Member

Choose a reason for hiding this comment

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

Why use np.arange here instead of range?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a section immediately after this example introducing range, but range as an iterator is a little more difficult to explain. We opted to compare range to np.arange, as the students should be familiar with this, and then lie a little about range

@@ -280,7 +271,7 @@
"\n",
"### Example - print integer information:\n",
"\n",
"- Write a program that asks the user to input an integer.\n",
"- Write a function that an integer as input.\n",
Copy link
Member

Choose a reason for hiding this comment

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

Write a function that takes an integer as input

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this, thanks for catching it.

@lgpage
Copy link
Member Author

lgpage commented Jan 31, 2018

@alchemyst and @smroux thanks for reviewing.

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.

Move "importing modules" after "name assignment and namespace"
3 participants