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

Adding CIFAR-10 DQN experiment #525

Merged
merged 11 commits into from
Dec 20, 2021

Conversation

mordred-skywalker
Copy link
Contributor

Reference issue

Contributes experiment related to #289

Type of change

Documentation - added experiment

What does this implement/fix?

This PR adds a Deep Q-learning experiment notebook on the task of image classification using CIFAR-10 dataset.

Additional information

mordred-skywalker and others added 5 commits December 13, 2021 05:01
- This commit adds a deep Q-learning experiment notebook on the task of image classification using CIFAR-10 dataset.
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #525 (e35eeb3) into staging (31b0f3d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           staging     #525   +/-   ##
========================================
  Coverage    90.09%   90.09%           
========================================
  Files            7        7           
  Lines          404      404           
========================================
  Hits           364      364           
  Misses          40       40           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31b0f3d...e35eeb3. Read the comment docs.

Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

You are mixing keras and tensorflow.keras here. The later one should be always used. Also, please rename notebook to cifar_q_learning.

Change the name of the file / tensorflow.keras as requested
@mordred-skywalker
Copy link
Contributor Author

You are mixing keras and tensorflow.keras here. The later one should be always used. Also, please rename notebook to cifar_q_learning.

Fixed!

Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

Not completely I think. You don't need to install keras from GitHub anymore right?

@mordred-skywalker
Copy link
Contributor Author

mordred-skywalker commented Dec 13, 2021

Not completely I think. You don't need to install keras from GitHub anymore right?

I don't think I have installed keras package in the notebook. Are you referring to the keras-rl2 package? That is an independent package for deep reinforcement learning algorithms. I am attaching the link here. (https://github.com/taylormcnally/keras-rl2)

You can also refer to Sarah's PR since we are using the same imported packages.

https://github.com/neurodata/ProgLearn/blob/staging/docs/experiments/Deep%20Q-learning%20in%20XOR-XNOR.ipynb

@PSSF23
Copy link
Member

PSSF23 commented Dec 13, 2021

@mordred-skywalker cool! Thanks for pointing that out. They use tensorflow.keras as well so it's all good. I will take another look over the notebook.

@mordred-skywalker
Copy link
Contributor Author

@mordred-skywalker cool! Thanks for pointing that out. They use tensorflow.keras as well so it's all good. I will take another look over the notebook.

@mordred-skywalker
Copy link
Contributor Author

@mordred-skywalker cool! Thanks for pointing that out. They use tensorflow.keras as well so it's all good. I will take another look over the notebook.

Sure! please let me know if other changes are required.

Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

A lot of imported packages are not used. You can remove them and make the import code block shorter. Other than that LGTM.

@PSSF23
Copy link
Member

PSSF23 commented Dec 14, 2021

Hmmm, it seems that you also need to work on GitHub skills as well, so that you don't delete & re-upload files each time, resulting in unnecessary commits. Try watching some video tutorials on how to commit & push your changes with GitHub Desktop or command line.

@mordred-skywalker
Copy link
Contributor Author

Hmmm, it seems that you also need to work on GitHub skills as well, so that you don't delete & re-upload files each time, resulting in unnecessary commits. Try watching some video tutorials on how to commit & push your changes with GitHub Desktop or command line.

Yes, I am relatively new to Github but I'll look into that :)

Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

There are still unnecessary imports like pandas, random, & shuffle. Look through each line and just search in your file. If you did not use it, discard it.

@PSSF23
Copy link
Member

PSSF23 commented Dec 14, 2021

Hmmm, it seems that you also need to work on GitHub skills as well, so that you don't delete & re-upload files each time, resulting in unnecessary commits. Try watching some video tutorials on how to commit & push your changes with GitHub Desktop or command line.

Yes, I am relatively new to Github but I'll look into that :)

You have been using it for a semester. Better make improvements before the next~

@mordred-skywalker
Copy link
Contributor Author

There are still unnecessary imports like pandas, random, & shuffle. Look through each line and just search in your file. If you did not use it, discard it.

Deleted them in the new commit.

@PSSF23 PSSF23 requested a review from jdey4 December 14, 2021 13:54
@jdey4 jdey4 merged commit dff232b into neurodata:staging Dec 20, 2021
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