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

Topic recognition #436

Open
wants to merge 26 commits into
base: topic-recognition
Choose a base branch
from

Conversation

jbart178
Copy link

pull request for Jackson Barton 45819061. No images in readme for outputs of model as model is still training will provide when they come out

@SiyuLiu0329
Copy link
Collaborator

This is an initial inspection, no action is required at this point

  • commit messages: some could be more detailed e.g. "something changed"
  • reconstruction: code seem OK, but no results preview or training logs, i am unable to verify the results
  • image gen: code seem OK, , but no results preview or training logs, i am unable to verify the results
  • readme: seems incomplete
  • code commenting: needs more work
  • no need to commit utils.py if its empty

@shakes76
Copy link
Owner

Good Practice (Design/Commenting, TF/Torch Usage)

Adequate use and implementation (training evidence lacking, unverifiable) -4
Good spacing and comments
Header blocks missing -1

Recognition Problem

Solves problem (can't verify because of lack of logs, generations etc.) -3
Driver Script present
File structure present
Shows Usage & Demo & Visualisation & Data usage (no plots, no outputs) -2
Module present
Commenting minimal -1
No Data leakage (prediction, testing evidence) -2
Difficulty: Hard

Commit Log

Meaningful commit messages, could be more informative -1
Progressive commits used

Documentation

ReadMe minimal, needs model details, PR should be verifiable -2
Good Description and Comments
Markdown used PDF submitted

Pull Request

Successful Pull Request (Working Algorithm Delivered on Time in Correct Branch)
No Feedback required, but not mergeable, no evidence that it is working correctly.
Request Description minimal, needs more info -2

@shakes76 shakes76 added the Unmergeable Cant be merged for history or other reason label Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unmergeable Cant be merged for history or other reason VQVAE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants