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

Fix issue 163 and 164 in 1_feature_extraction.ipynb #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gitgithan
Copy link

@gitgithan gitgithan commented Mar 21, 2023

3 changes in this notebook to fix #163 #164

  1. Quoting paths to prevent zsh shells erroring with zsh:1: no matches found: https://drive.google.com/uc?id=137RyRjvTBkBiIfeYBNZBtViDHQ6_Ewsp because it treats ? as glob character.
    image

  2. Convert cat.jpg that assumes user is running on colab into IMG_PATH that works on both colab and local
    image

  3. Make generator produce 68 * 128 instead of 68 * 32 images
    image

@sidgan
There are a few difficulties in me making PR on notebook files.

  1. No local GPU
    Later notebooks depend on data from previous notebooks. I have no GPU locally to produce those data. If I use colab for GPU, I have to add drive mounting code that messes up the notebook with extra cells. Current workflow is I generate necessary assets on colab then paste the changes individually from colab notebook to local notebook before commit. This means I never ran the local notebook top down, so there may be errors still.

  2. Cell output management
    It seems some cells have instructive output and some outputs are purposely deleted. It's best for the authors to run through the notebook again and decide what to show on the github version. I don't want to show my own filepaths in the output too.

  3. Diffing notebooks
    I tried ReviewNB but it gave a single chunk of red followed by a chunk of green because somehow it thinks the entire notebook changed. I eventually used nbdime locally, but if you know how I can use ReviewNB correctly, or any better way to communicate please advise.


Unaddressed issues

Outputs are still wrong

Found 8677 images belonging to 101 classes.
(8677, 'num images')
(67, 'num epochs')

At this cell i think it should show (68, 'num epochs') because ceil(8677/128) = 68 not 67. It looks like someone ran it without ceil first then added the ceil code without running again so the output was stale, which is another reason I hope authors can run the notebook and give a final check on outputs.

Python2 in metadata

  "metadata": {
    "kernelspec": {
      "display_name": "Python 2",
      "language": "python",
      "name": "python2"
    }

From json version of the ipynb we can see it is using python 2. The notebooks also open by default on python 2 in colab.
Changing it to python 3 may prevent frustration in users trying to debug a common issue, where syntax errors happen because python 2's print expected print 'hi' but the user think they're using python 3 and codes print('hi')

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.

chapter-4/1_feature_extraction.ipynb assumed running on colab
1 participant