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

Overhaul exercise for 2024 course #11

Merged
merged 38 commits into from
Aug 21, 2024
Merged

Overhaul exercise for 2024 course #11

merged 38 commits into from
Aug 21, 2024

Conversation

adjavon
Copy link
Contributor

@adjavon adjavon commented Jul 25, 2024

See issues #7 #8 #9 and #10.

The goal of this PR is to change the exercise so that the bulk of it works on a slightly modified version of MNIST that is classified by color instead of by digit.
Other changes include:

  • Adding an overview to the README
  • Using networks they have seen already in previous exercises, like the UNet and the DenseModel (from the failure modes exercise)
  • Simplifying the GAN training so that it can be done within the bounds of the exercise
  • Providing the scripts used to train the model weights that we provide them with
  • Modifying the text to build the story around this simpler problem
  • Adding synapse prediction/other biological problems as a bonus exercise

@adjavon
Copy link
Contributor Author

adjavon commented Jul 25, 2024

  • Add GitHub Action to build the notebook
  • Make sure that exercise doesn't overdo things that were done in the Intro DL exercise

@afoix afoix self-requested a review August 16, 2024 06:38
@afoix
Copy link
Contributor

afoix commented Aug 17, 2024

This comment is general feedback I find while checking the exercise. This is not to do with code but rather with the flow of the exercise.

Part 1

  • It seems that the image used in the README.md shows a Coloured MNIST dataset with classes that contain multiple colours. For example, the first two numbers in the top left belong to class "2" but are respectively a 5 yellow and a 0 red. How would you use the classes to classify by colors if they are not related?
Screenshot 2024-08-17 at 11 07 58
  • The dataset fails to completely download because of HTTP Error 403: Forbidden:
Screenshot 2024-08-17 at 11 08 25

Task 1.1: Load the classifier

  • In the loader classifier, we are assuming that they have this path: checkpoint = torch.load("extras/checkpoints/model.pth") and this file does not seem commited. (I've commented out the checkpoint loading for the sake of continuing with the review)
Screenshot 2024-08-17 at 11 22 07

Part 2

Task 2.1 Get an attribution

  • In the task 2.1 it looks like the attribution visualisation gives a black images only:
Screenshot 2024-08-17 at 11 26 54

Looks like it can be fixed with the following (normalising):

    #ax2.imshow(np.abs(attribution))
    ax2.imshow(np.abs(attribution)/np.abs(attribution).max())
Screenshot 2024-08-17 at 12 00 41
  • Maybe a separate plot for the adverserial loss as it's on a quite different scale as the other two? (or maybe it's fine, I guess this is just taste)
Screenshot 2024-08-17 at 12 13 24

Part4

Task 4: Create counterfactuals

  • I am not sure if the confusion matrix is expected to look like this:
Screenshot 2024-08-17 at 12 17 54

in function visualize_color_attribution_and_counterfactual

  • same visualisation issue as with task 2.1, can be fixed with ax2.imshow(np.abs(attribution)/np.abs(attribution).max())

In the readme

  • I believe the way to run jupyter lab is without having activated the environment in the first place. (We create the env, but we run jupyter lab from base which has it installed, unlike our env, and then this guy finds out our env for us to select in the GUI)
  • an extra dataset is mentioned in the readme as a "if time permits" exercise:
Screenshot 2024-08-17 at 12 24 38 It does not appear in the notebook.

Copy link
Contributor

@afoix afoix left a comment

Choose a reason for hiding this comment

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

Review done and comments added at the end of the PR 😊

We will be working with a simple example which is a fun derivation on the MNIST dataset that you will have seen in previous exercises in this course.
Unlike regular MNIST, our dataset is classified not by number, but by color!

![CMNIST](assets/cmnist.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

@adjavon I have a question regarding the image. You explained that the dataset classifies by color, however, the different classes contain different colors. Is that OK? What is the logic behind the classes? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the very end of the solutions notebook!! 😈 🌈 🙊

Ben-Salmon and others added 4 commits August 20, 2024 13:21
* review

* Commit from GitHub Actions (Build Notebooks)

---------

Co-authored-by: Ben Salmon <[email protected]>
Co-authored-by: Ben-Salmon <[email protected]>
* uses conda
* trains the classifier
@adjavon adjavon marked this pull request as ready for review August 20, 2024 18:38
@adjavon
Copy link
Contributor Author

adjavon commented Aug 20, 2024

Thanks @afoix for the review! Sorry about the setup issues. I've fixed almost everything that you found in the commits above. Some notes about what fixes what:

  • The setup file now uses conda and runs the classification so we can load the weights
  • The HTTPError seems to be just one of three possible URLs to download from & I've checked that the full dataset is downlaoded
  • The confusion matrix is normal again once the classifier is available
  • I've also cleaned up the README and the plotting.

The only thing missing is that I realized that the normalization you suggested is indeed necessary, but it breaks one of the points I'm making 🤯. All for the better though, since the resulting narrative makes more sense (also with the lecture). I'm working on that now and I'll ping you when it's done!!

As for the colors, yes, it's supposed to be more than one color per class! The answer to what makes the classes is hidden at the very end of the solution notebook 😉

@adjavon adjavon merged commit f0da865 into main Aug 21, 2024
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