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.

Sign up for GitHub

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

Jump to bottom

Provide script that downloads netflix prize data and puts it in the right place #56

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Provide script that downloads netflix prize data and puts it in the right place #56

wants to merge 4 commits into from

Conversation

JamesKohlsRepo
Copy link
Collaborator

@JamesKohlsRepo JamesKohlsRepo commented Dec 13, 2024 •
edited by audiodude
Loading

Fixes #43

@JamesKohlsRepo
issue 43
0eabc0c
@JamesKohlsRepo
issue 43
ac53fe9
@JamesKohlsRepo
Added clean_up_data and download_data
79e0df5 
these are intended to be run as pipenv scripts:
pipenv run download_data
pipenv run delete_data

they can also be run from file
@audiodude audiodude changed the title Issue 43 provide script that downloads netflix prize data and puts it in the right place Provide script that downloads netflix prize data and puts it in the right place Dec 13, 2024
audiodude
audiodude requested changes Dec 14, 2024
View reviewed changes
Copy link
Collaborator

@audiodude audiodude left a comment

Choose a reason for hiding this comment

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

This looks great!

When you merge main, go ahead and delete the Pipfile.lock file. Then merge your changes to Pipfile, and run pipenv install. That will regenerate a fresh Pipfile.lock. The file is machine generated and not intended to be merged by hand.

Pipfile
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file needs to be merged with main. It should be simple, just keep everything that main added, and add your new dependencies and scripts.

Pipfile Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
mediabridge/clean_up_data.py Outdated Show resolved Hide resolved
mediabridge/clean_up_data.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a bit overkill, because folks could just run rm -rf data to remove the data dir, and your download script will recreate it right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking in the future it could be used to delete specific data, but it's probably a YAGNI situation

mediabridge/download_data.py Outdated Show resolved Hide resolved
mediabridge/download_data.py Outdated Show resolved Hide resolved
audiodude
audiodude reviewed Dec 14, 2024
View reviewed changes
Pipfile Outdated Show resolved Hide resolved
@JamesKohlsRepo
Addressed comments
9a6b9f0 
Updated Pipfile to specify versions.
Updated Scripts definitions.
Moved scripts to process_data repository.
Edited header comment in download_data.
@audiodude
Copy link
Collaborator

Hey @JamesKohlsRepo!

I was going to try and resolve your merge conflicts for you so we could get this PR merged. However, it looks like you made your changes in your own fork. In the future, it might be a good idea to create branches in this repo directly (you're a contributor) so that other folks can work on the code too.

@jhanley634
Copy link
Collaborator

After merging down 72, this branch and PR should be deleted. Thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audiodude audiodude audiodude requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide script that downloads Netflix prize data and puts it in the right place
3 participants
@JamesKohlsRepo @audiodude @jhanley634