Skip to content
This repository has been archived by the owner on Oct 25, 2024. It is now read-only.

Novice notebooks updates #38

Merged
merged 15 commits into from
May 16, 2022
Merged

Novice notebooks updates #38

merged 15 commits into from
May 16, 2022

Conversation

matt-graham
Copy link
Contributor

Updates to the notebooks performed before last run of 'novice' course in February 2022

  • In 000Exemplar.ipynb notebook changes incorrect references to PNG image to JPEG image and removes use of BytesIO in reading images from bytes.
  • In 010Internet.ipynb notebook updates Google Maps URLs due to deprecation of previous API, adds more detail to description of URI scheme, minor updates to example usages of Requests library.
  • In 020TabularData.ipynb notebook adds extra description of tabular data formats and delimter-separated value files specifically and switches to using in-built csv module to read CSV rather than NumPy to avoid having to explain NumPy concepts before main NumPy introduction.
  • In 030StructuredData.ipynb removes !pip install pyyaml shell command in notebook to avoid OS compatibility issues and added exemplar maze structure from beginners course exercise for final exercise to allow for students who haven't done beginners course and so don't have own definition.
  • In 070Classes.ipynb small tweaks to remove explicit subclassing from object, use format strings rather than string concatenation and improve formatting of code / Markdown text.
  • In Makefile updates to list of files to clean to reflect new names.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

02-novice/000Exemplar.ipynb Show resolved Hide resolved
02-novice/000Exemplar.ipynb Show resolved Hide resolved
02-novice/010Internet.ipynb Show resolved Hide resolved
02-novice/020TabularData.ipynb Show resolved Hide resolved
02-novice/020TabularData.ipynb Show resolved Hide resolved
02-novice/020TabularData.ipynb Show resolved Hide resolved
02-novice/020TabularData.ipynb Show resolved Hide resolved
@@ -11,7 +11,7 @@
"cell_type": "markdown",
Copy link
Member

Choose a reason for hiding this comment

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

Way removed the only docstring we had? Yeah, weird we have only one... but it may be the useful one.

Also, on step the two prints were to distinguish before from after. Maybe the print in the middle could be added with a separator, like a ------ or something.


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I partly removed as it the previous docstring was using a plain string "..." rather than a multiline string """...""" , which while allowed and a valid docstring, is non-conventional . We also don't introduce docstrings previously at all I think so having one single example here without explanation I feel is more hindrance than help as I don't think it's clear this is a docstring and its not obvious why we would choose to add only this method. I've now updated to add docstrings to all the methods.

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.

Pretty good!! Some small comments left on the reviewNB. But approving it so you can merge it straight away if you use these suggestions.

@matt-graham
Copy link
Contributor Author

Pretty good!! Some small comments left on the reviewNB. But approving it so you can merge it straight away if you use these suggestions.

Thanks for the review and suggestions! Now made changes to address all of the comments.

@matt-graham
Copy link
Contributor Author

Build website job is failing with error

ImportError: cannot import name 'contextfilter' from 'jinja2'

which seems to be as the current version of nbconvert pinned in the requirements.txt file (nbconvert==5.6.1) doesn't include this fix for changes to the jinja2 API (see for example this related issue).

I will try pinning to a newer version with the fixed included.

@matt-graham
Copy link
Contributor Author

In the end trying to use an updated nbconvert version ended up hitting up against #19 and after going down a rabbit hole trying to fix that I've decided for now to just pin jinja2==3.0 which appears to fix the immediate issue, and delay sorting the broader issue of updating the build system dependencies to another PR...

@matt-graham matt-graham merged commit e0fefb3 into main May 16, 2022
@matt-graham matt-graham deleted the mmg/novice-notebooks-update branch May 16, 2022 16:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants